Rename IBFT networking classes #555
Conversation
|
|
||
| public interface PeerCollection { | ||
|
|
||
| void peerAdded(PeerConnection newConnection); |
There was a problem hiding this comment.
Shouldn't these both be in the present tense rather then the past tense? (addPeer and removePeer or add && remove)
| @@ -17,10 +17,9 @@ | |||
|
|
|||
| import java.util.Collection; | |||
|
|
|||
| public interface IbftMulticaster { | |||
| public interface ValidatorMulticaster { | |||
There was a problem hiding this comment.
Something doesn't sit right with this name, I suspect it should be ValidatorMulticast (not certain on the exact reason, it might be something grammar related)
There was a problem hiding this comment.
I figured classes are normally nouns, functions are normally verb-noun composition...
There was a problem hiding this comment.
Okay, I've found the reason; multicast is both a verb and a noun
https://en.oxforddictionaries.com/definition/multicast
When creating a compound noun (as we are here), then you have at least two options:
If you treat multicast as a noun:ValidatorMulticast
If you treat multicast as a verb ValidatorMulticaster
There was a problem hiding this comment.
As both are valid I'll suggest @jframe chime in on whether in this context multicast fits better as a verb or noun
There was a problem hiding this comment.
In my head, multicast is a verb ... but broadcast could clearly be a noun ... go figure.
There was a problem hiding this comment.
My preference is for ValidatorMulticaster. The name ValidatorMulticast doesn't seem right even though it might be valid from a grammar point of view.
There was a problem hiding this comment.
Alright, ValidatorMulticaster it is then 👍
| final Collection<Address> validators = validatorProvider.getValidators(); | ||
| sendMessageToSpecificAddresses(validators, message); | ||
| } | ||
|
|
||
| @Override | ||
| public void multicastToValidatorsExcept( | ||
| final MessageData message, final Collection<Address> exceptAddresses) { | ||
| public void send(final MessageData message, final Collection<Address> blackList) { |
There was a problem hiding this comment.
I like the rename to blacklist 👍
| @@ -17,10 +17,9 @@ | |||
|
|
|||
| import java.util.Collection; | |||
|
|
|||
| public interface IbftMulticaster { | |||
| public interface ValidatorMulticaster { | |||
There was a problem hiding this comment.
My preference is for ValidatorMulticaster. The name ValidatorMulticast doesn't seem right even though it might be valid from a grammar point of view.
| final Address peerAddress = removedConnection.getPeer().getAddress(); | ||
| peerConnections.remove(peerAddress); | ||
| } | ||
|
|
||
| @Override | ||
| public void multicastToValidators(final MessageData message) { | ||
| public void send(final MessageData message) { |
There was a problem hiding this comment.
Using send is nice, much simpler.
|
|
||
| import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; | ||
|
|
||
| public interface PeerCollection { |
There was a problem hiding this comment.
Not so sure about this name, it's not really a collection.
| @@ -125,10 +125,10 @@ public static TestContext createTestEnvironment( | |||
| final KeyPair nodeKeys = networkNodes.getLocalNode().getNodeKeyPair(); | |||
|
|
|||
| // Use a stubbed version of the multicaster, to prevent creating PeerConnections etc. | |||
| final StubIbftMulticaster stubbedNetworkPeers = new StubIbftMulticaster(); | |||
| final StubValidatorMulticaster stubbedMulticaster = new StubValidatorMulticaster(); | |||
There was a problem hiding this comment.
nit: this could be just called multicaster, the stubbed quality 'should' be irrelevant i.e. there's no need to know it's no the real thing (good ol' polymorphism)
| @@ -35,7 +35,7 @@ | |||
|
|
|||
| /** Class responsible for rebroadcasting IBFT messages to known validators */ | |||
| public class IbftGossip { | |||
There was a problem hiding this comment.
As the IbftMulticaster has been renamed, perhaps the IBFT prefix should be dropped and changed to include validator (as that's who being gossiped with?), ValidatorGossiper?
No description provided.