-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix: Revert "feat: shard aware peer management (#2151)" #2312
Conversation
This reverts commit dba9820. We need to revert this commit because the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f Also, the following js-waku test fails due to this commit: "same cluster, different shard: nodes connect"
You can find the image built from this PR at
Built from fa7218c |
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.
LGTM, thanks so much!
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.
LGTM! Thanks for going after steadily.
Mind elaborating on why this is causing issues? I would like to fully understand where the problem is before reverting. Not sure "the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them" is enough as a reason. waku-simulator is not using cluster-id, perhaps that can be related. that may be impacting peer management strategy. But ofc, even without that, it should work. created a tracker for that, slightly related. |
Thanks for the comments @alrevuelta ! This is precisely the actual reason: we cannot deliver things that are not backward-compatible. That change made the current All the breaking changes should be properly documented and informed. If we make a big change we need to ensure a smooth transition to users in the same way we keep deprecated I didn't test this issue in Another reason to justify that "revert" decision is the following issue from Let me know if need more details :) Cheers |
This reverts commit dba9820. We need to revert this commit because the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f Also, the following js-waku test fails due to this commit: "same cluster, different shard: nodes connect" * waku_lightpush/protocol.nim: minor changes to make it compile after revert
Description
This reverts commit dba9820.
We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f
Also, the following js-waku test fails due to this commit: "same cluster, different shard: nodes connect"
This approach is preferred because the #2151 PR was quite complex and I think is better to take a temporary
step back, re-think and re-test it until we make sure the
waku-simulator
and alljs-waku
tests work.