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

feat: autosharding content topic in config #1856

Merged
merged 2 commits into from
Aug 1, 2023
Merged

feat: autosharding content topic in config #1856

merged 2 commits into from
Aug 1, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jul 19, 2023 •
edited
Loading

Description

Added content topics in waku external config (pubsub VS content is now explicit). When used, a shard is picked and ENR is updated. Pubsub topics can also be used at the same time but only on the same cluster.

I also remove the default pubsub topic from config. Do we want to keep it until autoshard is working properly?

Let's not forget to deprecate the default pubsub topic when autosharding is 👍

Changes

  • Added pubsub & content topic to external config.
  • Update ENR based on all topics.

Tracking #1846

@SionoiS SionoiS changed the title feat: content topic autosharding feat: autosharding content topic in config Jul 20, 2023
@SionoiS SionoiS force-pushed the integrate branch 3 times, most recently from 28e8f0f to 3b63b17 Compare August 1, 2023 13:10
@SionoiS SionoiS marked this pull request as ready for review August 1, 2023 13:11
@SionoiS
- added content topic to config
83c47e0 
- updated ENR building to include content topics
vpavlin
vpavlin approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

LGTM

I feel like we may not need to keep the default, but we need to make sure all the docs/how-tos are updated and we mention it in release notes, from this point, it might be easier to keep the default topic until we switch fully to autosharding.

jm-clius
jm-clius approved these changes Aug 1, 2023
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.

LGTM. Note comment on keeping default pubsub topic for now. We should announce deprecation in following release in upcoming release notes.

apps/wakunode2/external_config.nim Outdated Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 1, 2023

I restored the default pubsub topic.

Where would we track deprecation notices for the release notes @vpavlin ?

@vpavlin
Copy link
Member

vpavlin commented Aug 1, 2023

I am thinking we should have a label for things we want to have explicitely mentioned in release notes - WDYT?

@SionoiS
Copy link
Contributor Author

SionoiS commented Aug 1, 2023 •
edited
Loading

I am thinking we should have a label for things we want to have explicitely mentioned in release notes - WDYT?

Yes that's good. I'll edit the top and mention the deprecation.

edit: I added the label @vpavlin

edit2: Might be better to use a "release note" label....

@SionoiS SionoiS merged commit afb93e2 into master Aug 1, 2023
14 of 15 checks passed
@SionoiS SionoiS deleted the integrate branch August 1, 2023 20:01
@vpavlin
Copy link
Member

vpavlin commented Aug 2, 2023

edit2: Might be better to use a "release note" label....

Yeah, that make sense, that way we can flag various issues/PRs and then decide what/how to mention in release notes

@jakubgs
Copy link
Contributor

jakubgs commented Aug 2, 2023

Please communicate infra breaking changes to the infra team before merging.

@jakubgs jakubgs mentioned this pull request Aug 2, 2023
Merged
jakubgs added a commit to status-im/infra-nim-waku that referenced this pull request Aug 2, 2023
@jakubgs
requirements: update nim-waku role, fix --topic flag
8c5e7c3 
Change caused by:
waku-org/nwaku#1856

Signed-off-by: Jakub Sokołowski <jakub@status.im>
jakubgs added a commit to status-im/infra-role-nim-waku that referenced this pull request Aug 2, 2023
@jakubgs
docker: change --topic flag to --pubsub-topic
0de1508 
Topic flag name change caused by:
waku-org/nwaku#1856

Signed-off-by: Jakub Sokołowski <jakub@status.im>
jakubgs added a commit to status-im/infra-status-legacy that referenced this pull request Aug 2, 2023
@jakubgs
requirements: update nim-waku role, fix --topic flag
e26879a 
Change caused by:
waku-org/nwaku#1856

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@jakubgs
Copy link
Contributor

jakubgs commented Aug 2, 2023

I have made the necessary changes:

@SionoiS SionoiS self-assigned this Aug 2, 2023
@SionoiS SionoiS mentioned this pull request Aug 4, 2023
Closed
3 tasks
@vpavlin vpavlin added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label Aug 9, 2023
@chaitanyaprem chaitanyaprem mentioned this pull request Aug 9, 2023
Closed
5 tasks
@fryorcraken fryorcraken added E:1.2: Autosharding for autoscaling See https://github.com/waku-org/pm/issues/65 for details and removed E:2023-10k-users labels Sep 8, 2023
@chair28980 chair28980 mentioned this pull request Nov 27, 2023
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpavlin vpavlin vpavlin approved these changes

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

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

@NagyZoltanPeter NagyZoltanPeter Awaiting requested review from NagyZoltanPeter

@alrevuelta alrevuelta Awaiting requested review from alrevuelta

Assignees

@SionoiS SionoiS

Labels
E:1.2: Autosharding for autoscaling See https://github.com/waku-org/pm/issues/65 for details release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SionoiS @vpavlin @jakubgs @jm-clius @fryorcraken