chore: deprecating named sharding #2723

gabrielmer · 2024-05-23T14:57:22Z

Description

Deprecating named sharding.

We won't allow further usage of named sharding and the default pubsub topic will be replaced by each cluster's 0 shard.

Another PR will follow in which we will deprecate the --pubsub-topic CLI configuration in favor of shard

Note

This PR will be merged after having announced the deprecation of named sharding during the next two releases (it will enter to v0.31.0). However, the general approach should be reviewed from now.

Issue

closes #2772

github-actions · 2024-05-23T14:57:37Z

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.

github-actions · 2024-05-23T15:01:52Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2723-rln-v1

Built from cd9ac42

github-actions · 2024-05-23T15:02:01Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2723-rln-v2

Built from cd9ac42

Ivansete-status

Thanks for it! 🥳
I'm not approving yet because it is in draft
Cheers

tests/testlib/wakunode.nim

waku/waku_core/topics/pubsub_topic.nim

Ivansete-status · 2024-05-29T07:35:15Z

waku/factory/external_config.nim

@@ -300,12 +300,13 @@ type WakuNodeConf* = object

    pubsubTopics* {.
      desc: "Default pubsub topic to subscribe to. Argument may be repeated.",
+      defaultValue: @[],


Shall we mark this pubsubTopic config attribute as deprecated? In that case, we will need to keep it for two more versions.

Yes, the idea is to deprecate it and stop creating confusion/overlap between pubsubTopic and shard.
So we will have to keep it for 2 more releases (announcing the deprecation in the release notes) and afterwards create a new PR deprecating it

Perhaps add Deprecated in the desc?

SionoiS · 2024-05-30T21:00:54Z

Should we take this opportunity to remove the usage of strings for pubsub topics internally?

My idea would be to replace PubsubTopic = string with a proper type that can the turned into a formatted string if needed.

WDYT?

alrevuelta · 2024-05-31T08:57:12Z

waku/factory/node_factory.nim

@@ -117,10 +117,16 @@ proc setupProtocols(
  ## Optionally include persistent message storage.
  ## No protocols are started yet.

+  var shardCount: uint32


Not sure about this. imho pubsubTopics.len shall be always >0?

pubsubTopics will be deprecated soon and this will be eg a cli flag shard-count or network-shards. but that's for another PR.

True! That was done because it made everything work better when I tested not setting anything in pubsub-topic and only setting shard.

But if we currently need to use pubsub-topic to list all the topics in the network, then it makes sense to add the logic similar to this in the next PR, the one deprecating pubsub-topic

That was done because it made everything work better when I tested not setting anything in pubsub-topic and only setting shard.

mm IMHO pubsub-topic has to be always set. It could be a default value (like when using clusterid=0 or 1) but it has to be set somewhere.

alrevuelta · 2024-05-31T08:59:18Z

waku/factory/waku.nim

@@ -96,12 +96,6 @@ proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =

  case confCopy.clusterId

-  # cluster-id=0


why removing cluster-id=0 configuration?

maybe we should keep it but with just 1 shard? for backwards compatibility? unsure though if someone is using it.

I mean, cluster-id=0 currently was a single named "shard" (default pubsub topic). From an app PoV that is equivalent to autosharding with a single shard.

My understanding is that after deprecating named sharding, cluster 0 was going to stop being a "special case".

Is there's any reason why we would want to keep the configuration of cluster 0 hardcoded? Once named sharding is deprecated, there won't be backward compatibility in any case

im ok with removing it. you are right with cluster 0 not being a special case anymore. But think also about the "known clusters" concept. Cluster configuration is not only about topics but also bootstrap nodes, validation strategy, etc.

so the question here is. do we want cluster-id=0 to be a "known cluster" with some specific configuration that people can use, or we don't care about that? imho nope, i would care only about TWN=1

but ccing @jm-clius

For now I think we should only care about TWN=1. Everything else is (equally) undefined.

gabrielmer · 2024-05-31T14:09:21Z

Should we take this opportunity to remove the usage of strings for pubsub topics internally?

My idea would be to replace PubsubTopic = string with a proper type that can the turned into a formatted string if needed.

WDYT?

I like the idea! I think it's better to do it in a separate PR once named sharding is deprecated so we don't mix different things and make the PR too big

jm-clius

Long overdue! Thanks.

jm-clius · 2024-05-31T16:57:45Z

waku/factory/external_config.nim

@@ -300,12 +300,13 @@ type WakuNodeConf* = object

    pubsubTopics* {.
      desc: "Default pubsub topic to subscribe to. Argument may be repeated.",
+      defaultValue: @[],


Perhaps add Deprecated in the desc?

gabrielmer · 2024-05-31T18:28:32Z

Perhaps add Deprecated in the desc?

How? Like this?

desc: "Deprecated. Default pubsub topic to subscribe to. Argument may be repeated.",

We can, although I will remove the pubsubTopics configuration in the next PR and both will be merged at the same time (roughly in 2 months), so the change won't really make it to master in practice. Now that I think about it, adding a default value to it also will never make it, I should probably remove that change :))

apps/chat2/config_chat2.nim

apps/chat2bridge/config_chat2bridge.nim

apps/liteprotocoltester/README.md

docs/operators/how-to/run.md

examples/cbindings/waku_example.c

waku/factory/external_config.nim

waku/factory/waku.nim

github-actions · 2024-07-02T13:29:24Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2723

Built from 71e06e0

Ivansete-status

LGTM! Thanks! 💯

…than configured

github-actions bot assigned gabrielmer May 23, 2024

gabrielmer added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label May 23, 2024

Ivansete-status reviewed May 29, 2024

View reviewed changes

gabrielmer force-pushed the chore-deprecating-named-sharding branch from b512631 to 2f36607 Compare May 30, 2024 11:47

gabrielmer marked this pull request as ready for review May 30, 2024 12:52

gabrielmer requested review from SionoiS, alrevuelta, NagyZoltanPeter, vpavlin, jm-clius, darshankabariya, richard-ramos and chaitanyaprem May 30, 2024 12:52

alrevuelta reviewed May 31, 2024

View reviewed changes

jm-clius approved these changes May 31, 2024

View reviewed changes

gabrielmer force-pushed the chore-deprecating-named-sharding branch 3 times, most recently from 65cb4f5 to 32675a8 Compare June 10, 2024 12:32

Ivansete-status added this to the v0.31.0 milestone Jun 25, 2024

fryorcraken reviewed Jun 28, 2024

View reviewed changes

fryorcraken mentioned this pull request Jun 28, 2024

doc: clarify max shard value on CLI args #2856

Closed

gabrielmer force-pushed the chore-deprecating-named-sharding branch from 32675a8 to 519a119 Compare July 2, 2024 13:24

Ivansete-status approved these changes Jul 9, 2024

View reviewed changes

gabrielmer added 13 commits July 9, 2024 17:38

initial changes for named sharding deprecation

2d2d98b

fixing compilation errors

68bdf0c

setting default pubsub topic value and modifying log

2d77a34

fix test

7aa99bd

fixing tests

e71c9ab

subscribe by default to shard 0

1106d68

Not allowing to use pubsub topics corresponding to different cluster …

b7d1b6c

…than configured

fix pubsub topic in defaultTestWakuNodeConf

56c539a

fixing comment

31c7f42

fixing redundant funcion types

790bdf3

implementing comments

4da0eaa

implementing feedback

46de574

logging changes

4565042

gabrielmer force-pushed the chore-deprecating-named-sharding branch from 61c7815 to 4565042 Compare July 9, 2024 14:39

gabrielmer merged commit e1518cf into master Jul 9, 2024
8 of 10 checks passed

gabrielmer deleted the chore-deprecating-named-sharding branch July 9, 2024 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: deprecating named sharding #2723

chore: deprecating named sharding #2723

gabrielmer commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

Ivansete-status left a comment

Ivansete-status May 29, 2024

gabrielmer May 30, 2024

jm-clius May 31, 2024

SionoiS commented May 30, 2024

alrevuelta May 31, 2024

gabrielmer May 31, 2024

alrevuelta Jun 3, 2024

alrevuelta May 31, 2024

gabrielmer May 31, 2024

alrevuelta Jun 3, 2024

jm-clius Jun 4, 2024

gabrielmer commented May 31, 2024

jm-clius left a comment

jm-clius May 31, 2024

gabrielmer commented May 31, 2024

github-actions bot commented Jul 2, 2024 •

edited

Loading

Ivansete-status left a comment

		@@ -96,12 +96,6 @@ proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =

		case confCopy.clusterId

		# cluster-id=0

chore: deprecating named sharding #2723

chore: deprecating named sharding #2723

Conversation

gabrielmer commented May 23, 2024 • edited Loading

Description

Note

Issue

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024 • edited Loading

github-actions bot commented May 23, 2024 • edited Loading

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer commented May 31, 2024

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer commented May 31, 2024

github-actions bot commented Jul 2, 2024 • edited Loading

Ivansete-status left a comment

Choose a reason for hiding this comment

gabrielmer commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading