This repository has been archived by the owner on Aug 2, 2021. It is now read-only.
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.
network: Kademlia Load Balancing #1774
network: Kademlia Load Balancing #1774
Changes from 23 commits
0f31b5a
c1c9ac5
fc65d5a
7dc9568
ca11c52
97ab47e
f0fc99d
0941717
e9263d7
ed1f9a9
904e204
29303f1
e53ae25
f82db7f
0e346e6
5665ea9
4c81a24
5b44ab3
52dacb0
3301920
49e7d09
ad5eac5
e78e2b3
21a0d2f
c615cdd
340ce15
96a8245
697b049
5cb6d46
abc51ab
9f951d1
d8f8059
c34062c
e7eefaf
3019822
2879509
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@kortatu Please have a look at @janos implementation of a similar mechanism with
SubscribePull
function instorage/localstore/subscribe_pull.go
andSubscribeToNeighbourhoodDepthChange
inkademlia.go
.If you use a similar approach, far less state needs to be maintained and the subscription logic can be included directly in the
kademlia
file (i.e. if theSubscribe
function returns a notification channel and a stop function)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.
Great, I will take a look at it.
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.
I have read
SubscribeToNeighbourhoodDepthChange
andSubscribePull
, it is the initial implementation I used, but it has a problem, first, is not extracted to a resuable file, and also, it has the problem that the subscriber can not stop the subscription from their side.In PubSubChannel, whenever the subscriber wants to stop the subscription, the channel is marked for closing, but not actually closed (because the only one closing a channel should be the writer of the channel).
I think those two pieces of code should be migrated to PubSubChannel for code sanity.
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.
I'm not sure this little amount of code justifies being generalized into a component in its own
I'm not really following...
SubscribePull
returnsc <-chan Descriptor, stop func()
, wherestop func()
is a function to deregister the returned channel from the subscriptions list of channels that should be notified. Callingstop
stops the subscription from the side of the subscriber. Or maybe I'm missing something here?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.
If you stop a channel from the reader side and the writer tries to write, you get an error. A channel should be always closed by the writer.
Anyway, I still see the main reason to create PubSubChannel, if you don't see that this amount of code used in 3 places (already) must be extracted, maybe I have other threshold for code reuse.
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.
Which tests are testing this pubsub implementation? I see no tests in gopubsub package.
And I think that we have tests for
SubscribePull
andToNeighbourhoodDepthChange
.I am sorry if you feel offended. But, PR reviews because of any code change I think that it is ok to question any code or comment in PR review. Even, my questions are mainly about problems that you mentioned that you have found in other places.
I do not think that anybody has any objections on abstracting common functionality, I think that the abstraction implementation is in question here.
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.
I forgot to commit the test file.
I found the problem with closing the subscription channel making the kademlia load balancing tests. As the addition of peers is done asynchronously, it could happen that the Load Balancer unsubscribe the moment a peer is added to Kademlia. Maybe is just a test thing, but is more correct to not close the channel from the reader side.
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.
I see what you are referring to for SubscribeToNeighbourhoodDepthChange, but since the channel is removed in the same function and there is syncOnce protecting the close, there should be no possibility for both send to the closed channel and close of the closed channel. This can be done with additional channel, not to close the returned channel from the client side but with more complexity. I found that this approach solved both problems in a simpler way since there already is Kademlia.lock.
In localstore SubscribePush a different approach is made and the returned channel is not closed on client side, but that subscription has different functionality then SubscribeToNeighbourhoodDepthChange.
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.
So it is working because the user of the API (the code subscribing) is doing things "kindly". I think an interface should not rely on that. As a generic publish/subscriber channel, I think that my implementation is safer.
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.
I have nothing to add. Thanks.
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.
No need to initialise value, it is implicit.
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.
Yes, but id doesn't hurt to make it explicit, it is clear for the reader. Swarm code is full of false boolean initialisations.
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.
I disagree that it is better, it is obvious from the struct declaration.