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

feat: custom per-topic MsgIdFunction #465

Closed
wants to merge 1 commit into from

Conversation

Wondertan
Copy link
Contributor

An attempt to close #464

@@ -213,6 +213,7 @@ const (

type Message struct {
*pb.Message
ID string
Copy link
Contributor Author

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.

Copy link
Contributor Author

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 {
Copy link
Contributor Author

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.

@Wondertan
Copy link
Contributor Author

I will add a test once this or another solution is considered good.

Copy link
Collaborator

@vyzo vyzo left a 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.

@Wondertan
Copy link
Contributor Author

Ok, will do

@vyzo
Copy link
Collaborator

vyzo commented Dec 28, 2021

thank you!

@BigLep
Copy link

BigLep commented Jan 7, 2022

@vyzo : are we good to merge?

@vyzo
Copy link
Collaborator

vyzo commented Jan 7, 2022

no, this should be closed. We have discussed a better implementatio (see the issue) which will supersede this.

@BigLep BigLep mentioned this pull request Jan 7, 2022
@vyzo vyzo closed this Jan 7, 2022
@Wondertan
Copy link
Contributor Author

I am going to work over it on weekends.

@Wondertan Wondertan mentioned this pull request Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-topic MsgIdFunction
3 participants