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.

Sign up for GitHub

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

Jump to bottom

fix: Enable autosharding in any cluster #2505

Merged
merged 2 commits into from
Mar 13, 2024
Merged

fix: Enable autosharding in any cluster #2505

merged 2 commits into from
Mar 13, 2024

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Mar 5, 2024 •
edited
Loading

Description

  • Removes hardcoding of amount of shards.
  • Removes hardcoding of cluster-id with enabled autosharding.
  • This enables any cluster-id != 1 to i) have autosharding if desired and ii) have whatever amount of shards they want.

For example, this PR enables the following. Creating a cluster-id=10 with 4 shards, where the node only subscribed to 0, 1

./build/wakunode2\
  --rest=true\
  --log-level=DEBUG\
  --relay=true\
  --pubsub-topic=/waku/2/rs/10/0\
  --pubsub-topic=/waku/2/rs/10/1\
  --pubsub-topic=/waku/2/rs/10/2\
  --pubsub-topic=/waku/2/rs/10/3\
  --cluster-id=10\
  --shard=0\
  --shard=1

@alrevuelta alrevuelta requested a review from SionoiS March 5, 2024 16:42
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2505-rln-v2-false

Built from 46809e3

@SionoiS
Copy link
Contributor

SionoiS commented Mar 5, 2024

LGTM

We just need to add dynamic shard subscription like in Waku metadata.

@alrevuelta alrevuelta force-pushed the cluster-configurable branch 2 times, most recently from 39e125a to 5921c68 Compare March 12, 2024 12:31
@alrevuelta alrevuelta marked this pull request as ready for review March 12, 2024 14:51
fbarbu15
fbarbu15 approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

SionoiS
SionoiS approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you!

tests/waku_relay/test_wakunode_relay.nim Outdated
@@ -506,10 +506,12 @@ suite "WakuNode - Relay":

await node.start()
await node.mountRelay()
require node.mountSharding(1, 1).isOk
echo node.wakuSharding.getShard(DefaultContentTopic)
Copy link
Contributor

Choose a reason for hiding this comment

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

left overs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wops, fixed: c6a446b

waku/waku_enr/sharding.nim Outdated
clusterId: uint16
shardIds: seq[uint16]
clusterId*: uint16
shardIds*: seq[uint16]

func clusterId*(rs: RelayShards): uint16 =
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the clusterId and shardIds if they are no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure: c6a446b

jm-clius
jm-clius approved these changes Mar 12, 2024
View reviewed changes
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!

@fbarbu15 fbarbu15 mentioned this pull request Mar 13, 2024
Merged
@alrevuelta alrevuelta merged commit 5a22580 into master Mar 13, 2024
14 of 15 checks passed
@alrevuelta alrevuelta deleted the cluster-configurable branch March 13, 2024 09:58
This was referenced Mar 13, 2024
Closed
Open
Closed
@alrevuelta alrevuelta mentioned this pull request Mar 26, 2024
Merged
@alrevuelta alrevuelta mentioned this pull request Apr 18, 2024
Closed
1 task
@gabrielmer gabrielmer mentioned this pull request Jul 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SionoiS SionoiS SionoiS approved these changes

@fbarbu15 fbarbu15 fbarbu15 approved these changes

@jm-clius jm-clius jm-clius approved these changes

@gabrielmer gabrielmer Awaiting requested review from gabrielmer

@Ivansete-status Ivansete-status Awaiting requested review from Ivansete-status

Assignees

@alrevuelta alrevuelta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alrevuelta @SionoiS @fbarbu15 @jm-clius