Allow peer to be in Relay ONLY mode for a topic #208
Conversation
| // We do NOT allow subscription without Relay. | ||
| // Note that Join is not an instanteneous operation. It may take some time | ||
| // before it is processed by the pubsub main loop and propagated to our peers. | ||
| func (p *PubSub) Join(topic string, opts ...RelayOpt) (*Relay, error) { |
There was a problem hiding this comment.
Since Subscribe = Join + subscription specific stuff, we need to create a Relay object every time Join is called in order to reference count subscribers so we can announce we are leaving the topic if we have no more subscribers & "un-cancelled" RelayOnly objects pending.
However, this means that will create multiple Relay objects even if the PubSub client/peer directly calls Join(without calling Subscribe) more than once in order to become a Relayer. I'm not sure if that makes sense because a peer is either RelayOnly or not. Should I add a comment saying that Join should ONLY be called once by the client?
There was a problem hiding this comment.
We should have a single relay object and reference count; Subscribing increases the reference count, unsubscribe decreases.
In fact, I am not even sure we need a relay object at all, just a reference count in the topic map.
There was a problem hiding this comment.
In fact, I am not even sure we need a relay object at all, just a reference count in the topic map.
I don't even think we need a reference count at all. If we take the strategy from #198 and #184 (comment) then there should really only be one topic handle and that topic handle should be able to optionally be a relay.
There was a problem hiding this comment.
Yeah, that's a valid strategy.
| @@ -363,6 +363,145 @@ func TestGossipsubGossip(t *testing.T) { | |||
| time.Sleep(time.Second * 2) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Need to add some tests for Floodsub as well.
| func TestGossipsubGossipWithSomeRelayOnlyPeers(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| hosts := getNetHosts(t, ctx, 15) |
There was a problem hiding this comment.
Change number of hosts to 20 for consistency.
|
@aarshkshah1992 thanks so much for taking a stab at this! Brief initial thoughts (which I hope to expand on when I've got a little more time) are that this seems a little overkill. Granted this is likely because you did the natural thing and emulated what the rest of the code looks like, but since the relay option (as @vyzo and @raulk specified in #28) seems to really just be about not allocating the Subscription message buffer we could probably significantly shrink the scope. Perhaps it would make sense to just add a flag to subscription like @vyzo any thoughts? |
|
I don't think we need a flag in the subscription -- we can simply not have a subscription object at all when we are only relaying. |
@vyzo the flag helps cut down on the complexity of managing another struct (the relay struct) and adding many more lines of boilerplate code to pubsub. However, if you really prefer that approach I'm not going to block on it. |
|
That's not what I meant -- I don't think we need either a relay object or to instantiate a subscription object for this. |
|
Sorry to put a stick on the wheels, but I think #198 comes before this. Once that design is implemented, it will make marking a topic as relay only much more natural. |
|
Topic handles now exist. |
|
Starting work on this again now that the amazing work on Topic handlers has landed. |
|
Closing this in favour of #229 |
For #28
@aschmahmann As discussed, this PR will allow a peer to 'Relay' for a topic without having to 'subscribe'/'consume' messages for that topic. However, a 'Subscriber' will always be a
'Relayer'.
Please let me know if the tests make sense/we need more tests.