-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: System wide typed pub sub topic registry #32262
Conversation
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.
looking good, docs should also be updated
Something I've thought about during the weekend: There is a race between TTL shutdown and the janitor actor cleaning up, we could potentially do better by having an I'm-going-to-stop callback to the Topic that it calls just before shutting down and then have a retry spin if the name is still occupied but not in the registry where the actor will soon shut down. Not entirely sure it is worth it though. |
I think there are several other uncertainties with delivery via PubSub so it's ok with possible loss in that unlucky moment. |
d24a82b
to
8fe7aee
Compare
One race that I think would be good to address is the first publish race. I think, when you first publish to a topic that is inactive, there's a race between the message you send, and the first listing sent by the receptionist, and I'm pretty sure the message you send will nearly always win, so that means that first message sent to a topic that is inactive on the node, but has subscribers elsewhere in the cluster, will almost never make it to the subscribers. If we could stash publish messages until the first listing is received (since the first listing is always sent immediately as an acknowledgement of subscribing), I think that would solve it. |
We already have that in the typed group topic, so I think we can do that, but let's treat it as a separate issue. I created #32267 for that. |
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.
looking good
2ffc199
to
137a5b4
Compare
137a5b4
to
d50fba9
Compare
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.
Looking good, but I think I spotted an issue.
akka-actor-typed/src/main/scala/akka/actor/typed/pubsub/PubSub.scala
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.
update reference docs?
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.
LGTM after a minor doc comment
The identity of the topic is a tuple of the type of messages that can be published and a string topic name but it is recommended | ||
to not define multiple topics with different types and the same topic name. | ||
Topics can be looked up in the @apidoc[PubSub](akka.actor.typed.pubsub.PubSub$) registry, this way the same topic will be represented by the same actor for a whole | ||
actor system. If the topic has not yet been started. |
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.
Is that last sentence incomplete?
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.
Yea, halfway through a thought. Fixed now.
akka-actor-typed/src/main/scala/akka/actor/typed/pubsub/PubSub.scala
Outdated
Show resolved
Hide resolved
….scala Co-authored-by: Renato Cavalcanti <renato@cavalcanti.be>
On top of #32261
References #31053