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: Pubsub.SeenMessagesStrategy #9543

Merged

Conversation

smrz2001
Copy link
Contributor

@smrz2001 smrz2001 commented Jan 13, 2023

This PR closes #9542 – attempts to apply the fix for libp2p/go-libp2p-pubsub#502 to kubo.

Once an official pubsub release is available, this draft PR can be updated accordingly and marked ready for review.

cc @lidel @BigLep

sending [msg_1] with message ID [MsgId_1] at T0.000000s
received [msg_1] at T0.000726s
sending [msg_2] with message ID [MsgId_1] at T0.002784s
did not receive [msg_2] at T1.011944s
sending [msg_3] with message ID [MsgId_2] at T1.013189s
received [msg_3] at T1.014176s
sending [msg_4] with message ID [MsgId_1] at T9.913194s
did not receive [msg_4] at T10.924792s
sending [msg_5] with message ID [MsgId_1] at T10.930913s
did not receive [msg_5] at T11.944472s
sending [msg_6] with message ID [MsgId_2] at T11.950365s
received [msg_6] at T11.950365s
sending [msg_7] with message ID [MsgId_1] at T21.036650s
received [msg_7] at T21.037893s
sending [msg_8] with message ID [MsgId_3] at T21.039733s
received [msg_8] at T21.039733s
--- PASS: TestMessageSeenCacheTTL (21.66s)

Should also close #9554

@smrz2001 smrz2001 changed the title feat: expire messages from the cache based on last seen time feat: use updated pubsub time cache implementation Jan 13, 2023
go.mod Outdated
@@ -1,5 +1,7 @@
module github.com/ipfs/kubo

replace github.com/libp2p/go-libp2p-pubsub => github.com/smrz2001/go-libp2p-pubsub v0.0.0-20230110155724-04bfcf58514f
Copy link
Member

@lidel lidel Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYSA I've pushed this change, so we can see if CI passes against last commit from libp2p/go-libp2p-pubsub#513
(It is late Friday so will do proper review on Monday)

@smrz2001 In the future, you can point at an arbitrary commit from your own fork via:

$  go mod edit -replace github.com/libp2p/go-libp2p-pubsub=github.com/smrz2001/go-libp2p-pubsub@04bfcf58514f59bddb650fec5b2edac018c8c406

Copy link
Contributor Author

@smrz2001 smrz2001 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, @lidel!! I didn't know we could point to fork commits like that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix any CI issues that come up.

@lidel lidel self-assigned this Jan 13, 2023
@lidel
Copy link
Member

lidel commented Jan 16, 2023

Updated to latest master to include newly added GH version of CI jobs and see if this PR fixes issue described in #9554.

Will check on the result tomorrow.

@lidel lidel self-requested a review January 16, 2023 02:23
@smrz2001
Copy link
Contributor Author

@lidel, since @vyzo requested switching back to the original, "first seen" time cache implementation in the pubsub PR, I updated the kubo code to explicitly use the new, "last seen" implementation.

I could also make this configurable if you suggest. Regardless, I think it would be best to default to the new implementation for kubo, even if configurable.

I could also make it configurable in a follow up PR. Please let me know either way.

@smrz2001 smrz2001 force-pushed the feature/res-853-creating-ipfs-pr-for-pl-to-review branch from c8f5d2a to 609c31b Compare January 23, 2023 21:46
@smrz2001
Copy link
Contributor Author

@lidel, I see the "context deadline timeout" failure on (my previous) flaky pubsub tests that you mentioned might get fixed via this PR.

I'm going to try bumping the timeout to see if that helps. 1 second is probably too small.

@smrz2001 smrz2001 force-pushed the feature/res-853-creating-ipfs-pr-for-pl-to-review branch from 2bbcb26 to 5e33745 Compare January 23, 2023 23:11
@smrz2001 smrz2001 marked this pull request as ready for review January 24, 2023 00:33
@smrz2001
Copy link
Contributor Author

@lidel, I see the "context deadline timeout" failure on (my previous) flaky pubsub tests that you mentioned might get fixed via this PR.

I'm going to try bumping the timeout to see if that helps. 1 second is probably too small.

Bah, it failed even with the larger value (though it passed just before that with the same value) :(

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also make this configurable if you suggest. Regardless, I think it would be best to default to the new implementation for kubo, even if configurable.
I could also make it configurable in a follow up PR. Please let me know either way.

Fine to change the default, but please include a configuration option in this PR – @vyzo makes a good point, there may be people who build things based on the old behavior

We should document the change in release notes, allowing them to switch back via optional configration option.

Let's make it future-proof, in case more advanced strategies will be added in the future.
Add *OptionalString to config/pubsub.go, and docs/config.md, something like:

+SeenExpirationStrategy *OptionalString  `json:",omitempty"`

With last-observed being the new implicit default and first-observed being the old one (feel free to pick better names). 🙏

@smrz2001
Copy link
Contributor Author

@lidel, I just pushed some updates to make the cache implementation configurable. I relied on the enum I added to the pubsub code and used an OptionalInteger for the new config (which for now would be 0 (first seen) or 1 (last seen)). Please let me know if I need to change this. I can just add strings that map to the enum values also.

@smrz2001
Copy link
Contributor Author

@lidel, I just pushed some updates to make the cache implementation configurable. I relied on the enum I added to the pubsub code and used an OptionalInteger for the new config (which for now would be 0 (first seen) or 1 (last seen)). Please let me know if I need to change this. I can just add strings that map to the enum values also.

Ok, nvm, I changed it to a string.

@BigLep BigLep requested a review from lidel January 26, 2023 16:00
@BigLep
Copy link
Contributor

BigLep commented Jan 26, 2023

@smrz2001 : FYI that @lidel is going to prioritize the review here so that we can wrap this up today/tomorrow and do 0.18.1 tomorrow: #9579 . If you can please be quick on any responses that come from @lidel , that would be great. Thanks!

@lidel : I assume we'll do a changelog addition. 0.18.1 changelog has been started in #9587

@smrz2001
Copy link
Contributor Author

Thanks so much for the cleanup and tweaks, @lidel 🙏🏼

Copy link
Member

@lidel lidel 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 @smrz2001 for driving this and being patient with us.
Added some docs and release notes, merging.

0.18.1 release can be tracked in #9579 (ETA Friday or Monday, depends if other item lands on time)

@lidel lidel merged commit 9652f24 into ipfs:master Jan 26, 2023
@lidel lidel changed the title feat: use updated pubsub time cache implementation feat: Pubsub.SeenMessagesStrategy Jan 26, 2023
@smrz2001 smrz2001 deleted the feature/res-853-creating-ipfs-pr-for-pl-to-review branch January 26, 2023 23:52
galargh pushed a commit that referenced this pull request Jan 30, 2023
* feat: expire messages from the cache based on last seen time
* docs: Pubsub.SeenMessagesStrategy

Ref. libp2p/go-libp2p-pubsub#513

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
3 participants