-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement PartitonHub #3287
Implement PartitonHub #3287
Conversation
|
||
#region internal classes | ||
|
||
private interface IConsumerEvent { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't consider this a mistake, but I'm starting to think, that in the future, we should favor tagged unions over this approach - no in every case, but for sure in cases like several singletons and one type with a error field. Example:
struct ConsumerEvent
{
enum ConsumerEventType { Wakeup, Initialize, HubCompleted }
readonly ConsumerEventType Type;
readonly Exception Failure;
....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should do that.
src/core/Akka.Streams/Dsl/Hub.cs
Outdated
|
||
private sealed class HubCompleted : IConsumerEvent | ||
{ | ||
public Option<Exception> Failure { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we need an Option<>
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably wanted to make it explicit that it can be null/none ( from a public api point of view ), but since it’s private we don’t need it.
@marcpiechura It's good, we can merge it. Right now I'm at work, so I cannot restart tests failing tests, but they are not related to the changes in this PR. |
* Implement PartitionHub * port docs * fix formatting * API approval * address remarks
* Implement PartitionHub * port docs * fix formatting * API approval * address remarks
closes #3111