Guarantee at least 10 peers to broadcast #767
Merged
Conversation
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
earlbread
reviewed
Jan 20, 2020
limebell
force-pushed
the
feature/guarantee-broadcast
branch
from
2840235
to
90826dd
Compare
limebell
force-pushed
the
feature/guarantee-broadcast
branch
from
90826dd
to
f890919
Compare
earlbread
previously approved these changes
Jan 20, 2020
limebell
force-pushed
the
feature/guarantee-broadcast
branch
from
f890919
to
fd71502
Compare
limebell
force-pushed
the
feature/guarantee-broadcast
branch
from
fd71502
to
1f67ab8
Compare
Codecov Report
@@ Coverage Diff @@
## master #767 +/- ##
==========================================
+ Coverage 86.3% 86.48% +0.18%
==========================================
Files 225 225
Lines 19280 19363 +83
==========================================
+ Hits 16640 16747 +107
+ Misses 1438 1413 -25
- Partials 1202 1203 +1
|
limebell
force-pushed
the
feature/guarantee-broadcast
branch
from
1f67ab8
to
45013ed
Compare
longfin
reviewed
Jan 23, 2020
longfin
approved these changes
Jan 23, 2020
moreal
approved these changes
Jan 23, 2020
dahlia
reviewed
Jan 23, 2020
| - If there are less than 10 peers in the routing table, select all peers. | ||
| - If there are more than 10 peers in the routing table, | ||
| choose one from each bucket, and if the number is less than 10, | ||
| then select an additional peers so that the total is 10. |
There was a problem hiding this comment.
As it's not a manual or docs, rather than explaining its behavioral details, we'd better to write a short descriptive guarantee and its intention.
| @@ -248,10 +252,14 @@ public void BroadcastTestMessage(Address? except, string data) | |||
| public void BroadcastMessage(Address? except, Message message) | |||
| { | |||
| var peers = Protocol.PeersToBroadcast(except).ToList(); | |||
| var peersString = peers.Aggregate( | |||
| new StringBuilder(), | |||
| (builder, peer) => builder.Append($"{peer.Address.ToHex()}, ")).ToString(); | |||
There was a problem hiding this comment.
Can't the same thing be easily achieved using string.Join() method?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
If the number of total peers in routing table is less then 10, broadcast messages to all peers.
If not, pick one from each bucket, and pick additional peers which were not selected in first step until total peers count reach 10.
Closes #765.