-
Notifications
You must be signed in to change notification settings - Fork 881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NetworkDB create NodeID for cluster nodes #1936
Conversation
e940235
to
dca40c7
Compare
Codecov Report
@@ Coverage Diff @@
## master #1936 +/- ##
=========================================
Coverage ? 37.93%
=========================================
Files ? 137
Lines ? 27289
Branches ? 0
=========================================
Hits ? 10351
Misses ? 15667
Partials ? 1271
Continue to review full report at Codecov.
|
dca40c7
to
b2ef9fb
Compare
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.
How will this change impact a mixed cluster with existing nodes still using the NodeName to identify a node while an existing node upgraded with this change will be looking and populating nodeID field in the networkDB message ?
Will it have any impact ?
@@ -140,7 +140,7 @@ func (nDB *NetworkDB) handleNetworkEvent(nEvent *NetworkEvent) bool { | |||
nDB.Lock() | |||
defer nDB.Unlock() | |||
|
|||
if nEvent.NodeName == nDB.config.NodeName { | |||
if nEvent.NodeName == nDB.config.NodeID { |
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.
Especially such changes where previously learnt messages from a node (prior to upgrading with this change) would have used NodeName in the messages. After the upgraded software, this node will be using NodeID, while the previous states maintained by the peers will still have the older states with the nodeName. And this check will fail, which is not expected.
discussed offline with @mavenugo, in the commit there is no change in the protobuf but only in the local variables, also every restart/upgrade the nodes are always using unique identifiers. |
Separate the hostname from the node identifier. All the messages that are exchanged on the network are containing a nodeName field that today was hostname-uniqueid. Now being encoded as strings in the protobuf without any length restriction they plays a role on the effieciency of protocol itself. If the hostname is very long the overhead will increase and will degradate the performance of the database itself that each single cycle by default allows 1400 bytes payload Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
b2ef9fb
to
4427afb
Compare
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.
LGTM
Separate the hostname from the node identifier. All the messages
that are exchanged on the network are containing a nodeName field
that today was hostname-uniqueid. Now being encoded as strings in
the protobuf without any length restriction they plays a role
on the effieciency of protocol itself. If the hostname is very long
the overhead will increase and will degradate the performance of
the database itself that each single cycle by default allows 1400
bytes payload
Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com