Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically add default Akka.Streams HOCON #489

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Aug 29, 2024

Changes

This is in response to a customer issue where the following happened:

  1. Using StreamRefs
  2. Transmits a StreamRef to a node that has never called any materialization functions
  3. Serialization error due to missing serializer definitions.

In normal Akka.NET you'd have to fix this by manually appending ActorMaterializer.DefaultConfig() as a fallback. In Akka.Hosting we can just do this automatically at startup.

I added a reference to Akka.Streams in the default Akka.Hosting module. There's not a lot of value in making a custom Akka.Streams.Hosting package as "add this config" is the only method that would be in it. If there's a good reason why we shouldn't add it to the default Akka.Hosting package, please say so in a review.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

This is in response to a customer issue where the following happened:

1. Using `StreamRef`s
2. Transmits a `StreamRef` to a node that has never called any materialization functions
3. Serialization error due to missing serializer definitions.

In normal Akka.NET you'd have to fix this by manually appending `ActorMaterializer.DefaultConfig()` as a fallback. In Akka.Hosting we can just do this automatically at startup.
@Aaronontheweb Aaronontheweb added the akka-streams Akka.Streams-related configuration label Aug 29, 2024
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detailed my changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some C# modernization

@@ -11,6 +11,7 @@

<ItemGroup>
<PackageReference Include="Akka.DependencyInjection" Version="$(AkkaVersion)" />
<PackageReference Include="Akka.Streams" Version="$(AkkaVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to move this project to centralized package management

// add the default Akka.Streams configuration by default - hurts nothing, but
// ensures that StreamRefs work correctly out of the box in case users
// haven't attempted to materialize a stream yet
b.AddHocon(ActorMaterializer.DefaultConfig(), HoconAddMode.Append);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the HOCON here - it can be in Append mode because it's just the defaults - it can go at the end of the line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More C# modernization.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Arkatufus Arkatufus merged commit d7cf636 into akkadotnet:dev Aug 30, 2024
2 checks passed
@Aaronontheweb Aaronontheweb deleted the akka-streams-config branch August 30, 2024 14:29
Arkatufus added a commit to Arkatufus/Akka.Hosting that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
akka-streams Akka.Streams-related configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants