You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR removes the usage of default pubsub topic. It is not possible to mix named and static sharding, and the default pubsub topic is a named shard. With this PR, I'm using shard rs/16/32 as the default shard for all non-community messages.
This is somewhat problematic because it's a breaking change and compatibility between diff versions would be lost? not sure how to solve this. Perhaps the status.prod fleet should also listen to shard rs/16/32 before this PR is merged?
Question: in this PR i've noticed lines like community.Shard().TransportShard(), that do not check for nil return value of Shard(). Which contrasts with some other places in code, like SaveCommunity() or MarshalPublicAPIJSON() in protocol/communities package, that do this check.
This func will only return nil if Community.config is nil - but when can this happen?
This func will only return nil if Community.config is nil - but when can this happen?
I'm not sure when this specific condition will this happen, tbh, but something that can indeed happen is that o.config.Shard is nil which happens when no shard has been assigned to a community. Since this is such a common scenario, in TransportShard i do check if the shard is nil. That way I don't need to do a comparison for nil whenever .Shard() is used, since TransportShard will automatically know what to do
@cammellos, @vitvly
I did try to get around the backward compatibility issue with having some hardcoded nodes and it is problematic because even if we add some hardcoded peers, the fact that the messages get published on separate pubsub topics means that every message would have to get published on both the default pubsub topic, and in a static shard, meaning that there wouldn't be any bandwidth improvements. If anything, the bandwidth usage would increase even more due to duplicated messages being posted on separate pubsub topics.
Also the bootnodes currently cant subscribe to both named and static shards at same time, meaning that if we want to have harcoded nodes just to support backward compatibility, we can't reuse those from shards.test fleet and would require a separate fleet of nodes with a subscription to the default pubsub topic
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
richard-ramos
force-pushed
the
shard/32
branch
2 times, most recently
from
This PR removes the usage of default pubsub topic. It is not possible to mix named and static sharding, and the default pubsub topic is a named shard. With this PR, I'm using shard rs/16/32 as the default shard for all non-community messages.
This is somewhat problematic because it's a breaking change and compatibility between diff versions would be lost? not sure how to solve this. Perhaps the status.prod fleet should also listen to shard rs/16/32 before this PR is merged?