-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: Pubsub.SeenMessagesTTL #9372
Merged
+276
−62
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
186a314
feat: make pubsub seen messages cache ttl configurable
smrz2001 7a22190
docs(config): Pubsub.SeenMessagesTTL
lidel 77eead2
chore(docs): add more details for `Pubsub.SeenMessagesTTL`
smrz2001 4b8eafa
fix(test): add tests for `Pubsub.SeenMessagesTTL`
smrz2001 a62272f
go mod tidy
smrz2001 c97c89b
chore: clarify pubsub tests
lidel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 PR needs a basic regression test that ensures
Pubsub.SeenMessagesTTLset by the user is respected.Unsure what is the best way to test this, you'd need to set it too low and then observe duplicates?
@smrz2001 we have some prior art in
test/sharness/t0320-pubsub.shif that helps.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.
Hey @lidel, it took me a bit to get everything figured out and setup with
sharness. After playing around with it a bit, I'm realizing that there isn't really a good way to induce duplicate pubsub messages. The API (rightly) doesn't allow setting a message ID explicitly.The only option I can currently come up with is to use
tc netemvia command line or code to simulate network traffic duplication.Does this seems like a viable approach? Not sure what the restrictions are around what tests are/aren't allowed to do. I've used
tc netembefore so I can figure this out, if acceptable.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.
Thank you for looking into this. I hoped there is an easier way, but I guess we've hit an edge case on this one.
We already have some helpers for sharness in
./test/dependencies, such aspollEndpoint, so that is a potential path forward (golang helper is a bit more future-proof, could be reused when we move away from the sharness). Usingtc netemis also an option, if we can get it to work on CI.@marten-seemann @vyzo is there a better / less painful way to test
WithSeenMessagesTTL/ simulate duplicated messages?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'm thinking aloud here but I could also make a
libp2p-simtool undertest/dependenciesthat connects to libp2p like a normal node but is capable of sending spurious messages. Might be useful in other contexts too perhaps.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.
probably not, we do go out of our way to supress them.
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 and libp2p/go-libp2p-pubsub#502 both sound sensible 👍
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.
@lidel, would it be possible to do a regular Golang unit test here instead of adding to
sharness?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.
@lidel, after a lot of hair pulling, I was unable to set up a libp2p simulator but I was able to figure out how to write a Go test under
test/integrationthat does the job, IMO. Would it work to keep the test there? Let me know what you think.FYI, this test uncovered another pubsub/cache bug, which I will fix under this issue.
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.
Thank you for doing due diligence on this, yes, your approach is acceptable: this is the best we can do given tools at hand, and it still acts as an early warning if go-libp2p APIs change, or a regression is introduced.
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.
Excellent, thanks so much, @lidel!! 🙏🏼🙏🏼
I'll try to get the other bugs fixed soon as well, and will update the dependencies and tests accordingly.