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: sharded peer management Round 2 #2332

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jan 3, 2024

Description

Peer manager must track peers per shard. Each with a target number of in/out peers and then connect/prune connections accordingly.

This new feature is behind a config flag

N.B. The first version was reverted because of backward-compatibility problems. Named sharding is deprecated but not unsupported.

Changes

  • Per shard pruning of connections
  • Per shard management of relay in/out connections
  • Refactor/clean-up
  • config flag

Tracking #1940

@SionoiS SionoiS force-pushed the feat--sharded-peer-manager branch from bff6f97 to fee20c7 Compare January 3, 2024 20:49
@SionoiS SionoiS changed the title feat: sharded peer manager Round 2 feat: sharded peer management Round 2 Jan 3, 2024
Copy link

github-actions bot commented Jan 3, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2332

Built from 4b98690

@SionoiS SionoiS force-pushed the feat--sharded-peer-manager branch from 2e2613a to 0b3a534 Compare January 9, 2024 13:52
@SionoiS
Copy link
Contributor Author

SionoiS commented Jan 9, 2024

Adding randomized peer selection speed-up reaching steady-state relay connections when using waku-simulator.

@SionoiS SionoiS marked this pull request as ready for review January 9, 2024 16:18
@SionoiS
Copy link
Contributor Author

SionoiS commented Jan 23, 2024

I will gate the new feature behind a flag so that we can test this if needed.

Fix ip colocation limit

Added randomized peer selection
@SionoiS SionoiS force-pushed the feat--sharded-peer-manager branch from 0b3a534 to 91a2636 Compare January 24, 2024 15:45
Copy link

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

@SionoiS SionoiS added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label Jan 24, 2024
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Looks amazing! Thank you! Added a couple questions :))

waku/node/peer_manager/peer_manager.nim Show resolved Hide resolved
tests/test_peer_manager.nim Show resolved Hide resolved
@gabrielmer gabrielmer self-requested a review January 25, 2024 15:08
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much! 🔥

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Thanks. Lgtm in terms of introducing the same functionality as before but now hidden behind a feature flag. Two thoughts:

  • could we target this for testing in deployed waku-simulator network and waku.test. I think the testing here is important enough to warrant creating a new issue just to track this, given that peer management is so fundamental
  • although named shards should be deprecated, is there a way to reconcile the two peer management paradigms even before named shards disappear? For example, it seems possible to consider a uint32 hash of named shards as the unique index representation of that shard.

@SionoiS
Copy link
Contributor Author

SionoiS commented Jan 26, 2024

  • could we target this for testing in deployed waku-simulator network and waku.test. I think the testing here is important enough to warrant creating a new issue just to track this, given that peer management is so fundamental

It was tested with waku-simulator but I agree that deploying to a fleet is a good idea.

  • although named shards should be deprecated, is there a way to reconcile the two peer management paradigms even before named shards disappear? For example, it seems possible to consider a uint32 hash of named shards as the unique index representation of that shard.

I never though of that but yes it could work. I'll look into it.

edit: The only way it would make sense to me would be if the default pubsub topic was given a shard number but then it breaks backward compatibility and conflict with static shard namespace. Seams a lot of work for not much benefit.

@SionoiS SionoiS merged commit edca1df into master Jan 30, 2024
11 of 12 checks passed
@SionoiS SionoiS deleted the feat--sharded-peer-manager branch January 30, 2024 12:28
@SionoiS
Copy link
Contributor Author

SionoiS commented Jan 30, 2024

status-im/infra-waku#10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants