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

#659 Adaptable Discoverying Nodes #705

Merged
merged 2 commits into from
Dec 18, 2018

Conversation

spikeekips
Copy link
Contributor

@spikeekips spikeekips commented Nov 12, 2018

Adaptable Connecting Nodes

Resolves #659

At this time, a node must know these things about its validators,

  • all public addresses of nodes
  • all endpoints of nodes

When one of the endpoints is updated, all the nodes in the network must be restarted with the updated endpoints. This will be a painful job for maintanence.

This patch will solve this problem and new endpoint of validators will be updated without downtime.

Changes

--validators only with public address

The current way to set --validators is,

--validators "self https://127.0.0.1:2822?address=G..W&alias=node2 https://127.0.0.1:2823/?address=G..4&alias=node3 https://127.0.0.1:2824/?address=G..O&alias=node4"

The new way is,

--validators "self G..W G..4 G..O"

Node just knows only the public address, the variable information like endpoint and alias are removed.

The environment variable also can be set,

SEBAK_VALIDATORS="self G..W G..4 G..O"

New option, --discovery

--discovery https://1.2.3.4:5678 --discovery https://8.7.6.5:4321

Instead of setting endpoint in --validators option, using --discovery node will try to discover the validators. When a node starts, the node will send network.DiscoveryMessage to --discovery values. Each node sends and receives network.DiscoveryMessages, the node can be started until the number of discovered validators is over consensus threshold.

--discovery also can be set by an environment variable, SEBAK_DISCOVERY, multiple values can be separated by empty space.

Omitting -discovery option also be possible. Without --discovery node will just wait network.DiscoveryMessage from other nodes.

WatcherMode

Watcher does not care about the validator, it just needs the target node to watch. Watcher will use --discovery instead of --validators, the node address will be fetched from the node info of --discovery node. At this process, the remote node info is compared with watcher; the network id and initial balance are matched.

Minor changes

  • node.LocalNode.Endpoint() will return if publishEndpoint is set
  • node.LocalNode.ClearValidators() will make empty validators

Etc

This patch was severely tested in integration test :)

@spikeekips
Copy link
Contributor Author

The test cases will be added.

@spikeekips spikeekips self-assigned this Nov 12, 2018
@spikeekips spikeekips added the enhancement New feature or request label Nov 12, 2018
@Geod24
Copy link
Contributor

Geod24 commented Nov 20, 2018

There's a lot to test. And it needs a rebase. Could you update ? I'll focus on reviewing/testing this tomorrow so we can get it merged quickly.

@spikeekips
Copy link
Contributor Author

Thanks l, I will.

@spikeekips spikeekips force-pushed the 659-take1-connect-with-message branch 3 times, most recently from 1712e1f to d0b435a Compare December 16, 2018 11:28
@codecov-io
Copy link

codecov-io commented Dec 16, 2018

Codecov Report

Merging #705 into master will decrease coverage by 0.17%.
The diff coverage is 58.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
- Coverage   60.43%   60.25%   -0.18%     
==========================================
  Files         151      153       +2     
  Lines       10347    10708     +361     
==========================================
+ Hits         6253     6452     +199     
- Misses       3394     3522     +128     
- Partials      700      734      +34
Flag Coverage Δ
#integration_tests_long_term 44.3% <53.43%> (+0.08%) ⬆️
#integration_tests_node 40.36% <53.65%> (+0.64%) ⬆️
#unittests 48.65% <14.19%> (-1.37%) ⬇️
Impacted Files Coverage Δ
lib/network/memory.go 57.57% <ø> (+1.69%) ⬆️
lib/errors/errors.go 0% <ø> (ø) ⬆️
lib/common/message.go 31.57% <ø> (-10.53%) ⬇️
lib/network/http2.go 87.6% <ø> (-0.59%) ⬇️
lib/node/runner/api_node.go 77.17% <ø> (ø) ⬆️
lib/network/memory_client.go 33.33% <0%> (-16.67%) ⬇️
lib/sync/watcher.go 0% <0%> (ø) ⬆️
lib/node/runner/test.go 89.06% <100%> (-0.34%) ⬇️
lib/sync/fetcher.go 57.5% <100%> (ø) ⬆️
lib/node/validator.go 76.31% <100%> (+2.4%) ⬆️
... and 17 more

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 ae7345b...3f6f24c. Read the comment docs.

@spikeekips spikeekips changed the title #659 Adaptable Connecting Nodes #659 Adaptable Discoverying Nodes Dec 16, 2018
@spikeekips
Copy link
Contributor Author

Entirely newly rewrote. Please continue to review

Copy link
Member

@Charleslee522 Charleslee522 left a comment

Choose a reason for hiding this comment

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

👍 👍
And only have some minor issues.

lib/network/discovery_message.go Outdated Show resolved Hide resolved
lib/network/discovery_message.go Outdated Show resolved Hide resolved
lib/network/memory_client.go Show resolved Hide resolved

// startDiscovery will try to discover the validators; it will block until
// enough validators is discovered, and then it will keep discovering the left
// validators.
Copy link
Member

Choose a reason for hiding this comment

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

Good documentation 👍

lib/network/discovery_message.go Outdated Show resolved Hide resolved
lib/network/discovery_message.go Outdated Show resolved Hide resolved
lib/network/discovery_message.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kfangw kfangw left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@Charleslee522 Charleslee522 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@spikeekips spikeekips merged commit 5c5b761 into bosnet:master Dec 18, 2018
@Geod24 Geod24 deleted the 659-take1-connect-with-message branch January 18, 2019 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants