-
Notifications
You must be signed in to change notification settings - Fork 187
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: custom per-topic MsgIdFunction #465
Conversation
@@ -213,6 +213,7 @@ const ( | |||
|
|||
type Message struct { | |||
*pb.Message | |||
ID string |
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 allows msg id to be generated only once. Before it was generated 3 times in a pipeline, which can be expensive when hashing is used. Also, this simplifies code a bit and removes the requirement to MsgIdFunction into MessageCache.
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.
Fun fact: One of the Mina Protocol's motivations to rework their protocol is high CPU spikes on msg id generation.
@@ -1164,6 +1175,15 @@ type TopicOptions struct{} | |||
|
|||
type TopicOpt func(t *Topic) error | |||
|
|||
// WithCustomMessageIDFunc is an option to customize the way a message ID is computed for a pubsub message | |||
// within the Topic. By default, it uses global MsgIdFunction registered on a PubSub instance. | |||
func WithCustomMessageIDFunc(msgId MsgIdFunction) TopicOpt { |
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.
Not sure about a good naming here. Added a work Custom
to differ from global Pubsub option.
I will add a test once this or another solution is considered good. |
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.
See comment on issue.
Note that computing the ID once and caching it is valuabke on its own, you can cherry pick this for another pr.
Ok, will do |
thank you! |
@vyzo : are we good to merge? |
no, this should be closed. We have discussed a better implementatio (see the issue) which will supersede this. |
I am going to work over it on weekends. |
An attempt to close #464