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: msgIdGenerator #468

Merged
merged 10 commits into from
Jan 23, 2022
Merged

feat: msgIdGenerator #468

merged 10 commits into from
Jan 23, 2022

Conversation

Wondertan
Copy link
Contributor

@Wondertan Wondertan commented Jan 9, 2022

  • Introduces msgIdGenerator
  • Ensures msg id is generated once only
    • Besides Tracer methods rcving *RPC
  • Allows setting per-topic MsgIDFunction

Closes #464

Personally, I still prefer the solution, where we compute and set Message.ID once in the pipeline and then assume it is set everywhere, instead of calling generator everywhere, but I might be missing something

gossip_tracer.go Show resolved Hide resolved
mcache.go Show resolved Hide resolved
mcache.go Show resolved Hide resolved
mcache_test.go Show resolved Hide resolved
pubsub.go Show resolved Hide resolved
trace.go Outdated Show resolved Hide resolved
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.

Thank you, this looks pretty good!

Left a couple of comments suggesting some minor improvements and then it's ready to merge.

gossip_tracer.go Show resolved Hide resolved
gossip_tracer.go Show resolved Hide resolved
mcache.go Show resolved Hide resolved
mcache.go Show resolved Hide resolved
midgen.go Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Jan 13, 2022

Basically the one small thing that we can and should improve on is provide a raw ID method in idGen, so that we don't have to allocate a wrapper message to compute it in the tracer.

This method could simply be refactored out of ID, and used by it.

@aschmahmann aschmahmann added the need/author-input Needs input from the original author label Jan 14, 2022
@vyzo
Copy link
Collaborator

vyzo commented Jan 21, 2022

hey @Wondertan, when will you have some time to finish this? I would like to merge it soon.

It is very close, just needs to fix the allocation in the tracer by refactoring rawMsgInd.
It also needs a rebase.

@Wondertan
Copy link
Contributor Author

Hey @vyzo, planned for tomorrow.

@Wondertan
Copy link
Contributor Author

Rebased on master

@Wondertan
Copy link
Contributor Author

One last thing I am concerned about is naming for options.
WithMessageIdFn is global options
WithMsgIdFunction is for topic. Would be cool to unify them somehow or just append Topic in the end.

I will also add a test for the option later today.

@vyzo
Copy link
Collaborator

vyzo commented Jan 22, 2022

yeah, lets call it WithTopicMessageId or something similar.

@Wondertan
Copy link
Contributor Author

Ready.

Btw, why do you use real Swarm and not mocknet in tests? What does this give for testing?

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.

looks good, thank you!

@vyzo vyzo merged commit be065ce into libp2p:master Jan 23, 2022
@vyzo
Copy link
Collaborator

vyzo commented Jan 23, 2022

Btw, why do you use real Swarm and not mocknet in tests? What does this give for testing?

Well, we want some integration testing happening and observing effects of the actual network in use; mocknet would deny us that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-topic MsgIdFunction
3 participants