Skip to content
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

Implement Node Id on Server #1059

Merged
merged 5 commits into from
Dec 14, 2017
Merged

Implement Node Id on Server #1059

merged 5 commits into from
Dec 14, 2017

Conversation

no2chem
Copy link
Member

@no2chem no2chem commented Dec 11, 2017

Overview

Description: This PR introduces a node Id field, which is generated on first start and stored in the data store. The node Id is a 128-bit UUID which is represented as a Base64 URL-safe string. The planned use of a base64 string is to append it to a server url, in the form <host>:<port>:<node-id>, which will enable the node id to be checked at connection time during a handshake to be implemented in the future. Currently, the Node Id can be retrieved in using a versionRequest call to a server, and the server can retrieve the node Id from a serverContext.

Why should this be merged: This PR makes it possible to disambiguate servers which have failed, but restarted at the same IP address, eliminating configuration errors from allowing clients to connect to incorrect servers.

Related issue(s) (if applicable): Fixes #1050

Checklist (Definition of Done):

  • There are no TODOs left in the code
  • Coding conventions (e.g. for logging, unit tests) have been followed
  • Change is covered by automated tests
  • Public API has Javadoc

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 70.569% when pulling bd409ab on nodeIdd into bcca6e2 on master.

@no2chem no2chem added this to the 0.2.0 milestone Dec 12, 2017
Copy link
Member

@zalokhan zalokhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be one test to create a server and check the creation of the node id ?

@@ -275,12 +271,15 @@ public static void main(String[] args) {
// Create a common Server Context for all servers to access.
serverContext = new ServerContext(opts, router);

// Generate a node ID if necessary.
generateNodeId(serverContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be generated here?
Or can we move it down in the ServerContext creation.
This way we will have a similar behaviour between our tests and real servers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - moved it to server context. Going to wait for #1061 to make meaningful tests.

* @param context The server context to use.
*/
private static void generateNodeId(@Nonnull ServerContext context) {
String currentId = context.getDataStore().get(String.class, "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the setter and getter of this NODE_ID to the server context.
Every component will try to consume it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not needed now that there is a helper function.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.637% when pulling 1954c4a on nodeIdd into bcca6e2 on master.

Copy link
Member

@zalokhan zalokhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@corfudb-performance
Copy link
Collaborator

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 4ae0bbe.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #1059 Graphs

@codecov
Copy link

codecov bot commented Dec 14, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@c061e06). Click here to learn what that means.
The diff coverage is 78.37%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1059   +/-   ##
=========================================
  Coverage          ?   65.94%           
=========================================
  Files             ?      213           
  Lines             ?     9890           
  Branches          ?      987           
=========================================
  Hits              ?     6522           
  Misses            ?     2996           
  Partials          ?      372
Impacted Files Coverage Δ
.../org/corfudb/infrastructure/NettyServerRouter.java 54.66% <ø> (ø)
...n/java/org/corfudb/infrastructure/CorfuServer.java 0% <0%> (ø)
...in/java/org/corfudb/infrastructure/BaseServer.java 68.42% <100%> (ø)
...rg/corfudb/protocols/wireprotocol/VersionInfo.java 85.71% <66.66%> (ø)
...time/src/main/java/org/corfudb/util/UuidUtils.java 90.9% <90.9%> (ø)
...java/org/corfudb/infrastructure/ServerContext.java 95.83% <91.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c061e06...1f0ece1. Read the comment docs.

@no2chem no2chem merged commit b7ddb57 into master Dec 14, 2017
@no2chem no2chem deleted the nodeIdd branch December 14, 2017 12:26
no2chem added a commit that referenced this pull request Dec 14, 2017
* Implement Node Id as base64

* Add nodeId to version request

* Move node id generation to servercontext
no2chem added a commit that referenced this pull request Dec 14, 2017
* Implement Node Id as base64

* Add nodeId to version request

* Move node id generation to servercontext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants