-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add wait-for-subscribers feature #7652
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
Add wait-for-subscribers feature #7652
Conversation
…bers-feature # Conflicts: # src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/reference.conf
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/OverflowStrategy.cs
Outdated
Show resolved
Hide resolved
…us/akka.net into Add-wait-for-subscribers-feature
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.
Self-review
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
.ToImmutableDictionary(kv => kv.Key, kv => kv.Value); | ||
} | ||
} | ||
=> _registry |
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.
No logic change, just modernization
/// <param name="maxBufferedMessagePerTopic">Maximum message buffer size for each topic</param> | ||
/// <param name="bufferedMessageTimeoutCheckInterval">Buffered message timeout condition check interval</param> | ||
/// <exception cref="ArgumentException">Thrown if a user tries to use a <see cref="ConsistentHashingRoutingLogic"/> with routingLogic.</exception> | ||
public DistributedPubSubSettings( |
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.
New backward compatible .ctor
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubSettings.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
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.
Made some suggestions
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Outdated
Show resolved
Hide resolved
/// <param name="maxDeltaElements">The maximum number of delta elements that can be propagated in a single gossip tick.</param> | ||
/// <param name="sendToDeadLettersWhenNoSubscribers">When a message is published to a topic with no subscribers send it to the dead letters.</param> | ||
/// <exception cref="ArgumentException">Thrown if a user tries to use a <see cref="ConsistentHashingRoutingLogic"/> with routingLogic.</exception> | ||
[Obsolete("Use .ctor that supports WaitForSubscribers instead. Since 1.4.42")] |
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.
A thing we did on #7637 that we should do here:
- Don't even make the CTORs on these settings classes
public
. Force users to use thestatic SettingsClass.Create
method. Eliminates a ton of binary compatibility issues long-term. - Make all of these settings classes into
record
types - this eliminates the.WithValueName(
method spam.
Maybe we do that across the board in v1.6 - I'm doing it in v1.5 for the TcpSettings
type on #7637. I'm fine doing that for DistributedPubSub
too.
/// <param name="maxDeltaElements">The maximum number of delta elements that can be propagated in a single gossip tick.</param> | ||
/// <param name="sendToDeadLettersWhenNoSubscribers">When a message is published to a topic with no subscribers send it to the dead letters.</param> | ||
/// <exception cref="ArgumentException">Thrown if a user tries to use a <see cref="ConsistentHashingRoutingLogic"/> with routingLogic.</exception> | ||
[Obsolete("Use .ctor that supports WaitForSubscribers instead. Since 1.4.42")] |
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.
Implication of this design change is that users have to call DistributedPubSubSettings.Create(ActorSystem)
and then they can manipulate it from there.
/// <param name="maxDeltaElements">The maximum number of delta elements that can be propagated in a single gossip tick.</param> | ||
/// <param name="sendToDeadLettersWhenNoSubscribers">When a message is published to a topic with no subscribers send it to the dead letters.</param> | ||
/// <exception cref="ArgumentException">Thrown if a user tries to use a <see cref="ConsistentHashingRoutingLogic"/> with routingLogic.</exception> | ||
[Obsolete("Use .ctor that supports WaitForSubscribers instead. Since 1.4.42")] |
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.
Changing this on v1.5 will probably blow a bunch of stuff up immediately, so we should probably save that for v1.6
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
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.
Left you a suggestion on the pruning check
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
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.
More comments
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Internal/TopicMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
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.
Left some more suggestions but I think overall the design looks good - what's the testing plan?
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedMessages.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs
Show resolved
Hide resolved
{ | ||
if (message is PublishWithAck needAck) | ||
{ | ||
sender.Tell(new PublishFailed(needAck, PublishFailReason.Timeout)); |
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.
Do we want to do an early return here and skip logging the dead letter or do we want to do both? The way we have it does both and I actually think that's ok - since the DeadLetter
monitoring will usually get universal attention.
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 think its wise to change old behavior
} | ||
|
||
private void PublishToEachGroup(string path, Publish publish) | ||
private void PublishToEachGroup(string path, IWrappedMessage publish) |
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.
IMHO, do a common interface for both of the Publish
message types and handle that instead.
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.
This can't be done because PublishMessage
is also used to send SendAll
Fixes #7627
DistributedPubSub wait-for-subscriber feature flag
HOCON settings added
Settings added to DistributedPubSubSettings
Buffer
PublishWithAck
message and able to deliver it immediately, it will send back aPublishSucceeded
message back to the original sender as a signal.PublishWithAck
message, it will store the message as aBufferedMessage
inside the buffer.Buffer timeout
PublishWithAck
message contains the timeout value for each messagestimeout-check-interval
time, the mediator will scan through all of the buffered message to check and see if it has been sitting in the buffer for more than timeout period.PublishFailure
message will also be sent to the original sender to signal publish failures.Buffer delivery
NewBucketKeysAdded
message to itself.NewBucketKeysAdded
message is received, the mediator checks the buffer and re-send all waiting messages that matches the keys to itself.PublishSucceeded
message to the original sender as a signal.Buffer overflow
When a new message is inserted and the topic buffer count exceeds
buffered-messages.max-per-topic
, the mediator will discard the oldest message in the buffer and send aPublishFailed
to the original sender of that message.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):