-
Notifications
You must be signed in to change notification settings - Fork 61
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
doc: clarify max shard value on CLI args #2856
Conversation
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
Available types:
|
You can find the image built from this PR at
Built from c62a8f8 |
pubsubTopics.add($NsPubsubTopic.staticSharding(conf.clusterId, shard)) | ||
|
||
if conf.shards.len == 0: | ||
# If shards are not configured but content topics are, then assume autosharding is expected |
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 am guessing this is incorrect as you'd need to have shards configured (ie 0 to 7 ) to apply an auto sharding logic as the result of the autosharding logic depends on the shard range available.
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.
so currently we deduce the amount of shards based on the number of pubsub topic configured. In other words, the --pubsub-topic
parameter shows all the possible shards that exist
To avoid ambiguity, in the PR deprecating the pubsub topic concept will add a new CLI flag which will indicate the number of shards in a network
debug "Shards created from content topics", | ||
contentTopics = conf.contentTopics, shards = shards | ||
var pubsubTopics: seq[string] = conf.pubsubTopics | ||
for shard in conf.shards: |
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.
@gabrielmer I think this is needed in #2723
@@ -423,7 +423,7 @@ proc mountRelay*( | |||
|
|||
node.switch.mount(node.wakuRelay, protocolMatcher(WakuRelayCodec)) | |||
|
|||
info "relay mounted successfully" | |||
info "relay mounted successfully", pubsubTopics |
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.
Something useful @gabrielmer
I started this PR because it bugged me that I could not simply start
wakunode2
and connect to it viawakucanary
becausewakucanary
wants shards:In the good default value spirit, we should be able to run the binary with minimal config out of the box, without having to fiddle things.
The good default at the moment should either be:
1, TWN (aka RLN Relay protected network)
2. Status prod
There is value in doing either. However, for the sake of dogfooding and pushing RLN further, I believe (1) should be default. Until we can get RLN on Status network.
So, the proposal I'd like to make before I proceed forward is to change all default parameters to TWN, meaning that a user will have to pass a API address for eth client to play with nwaku.
This would mean something similar to nwaku-compose, but without postgres.
Cc @waku-org/nwaku-developers
cc @gabrielmer some suggestions in there in terms of handlng shards, in reference to #2163