Implement getting the cluster nodes list from nodes #135

msiomkin · 2019-03-21T11:56:00Z

Periodically call a user-defined Lua function on a node to obtain or
refresh the full cluster nodes list.

Resolves: #134

Totktonada

Thanks for the patch and sorry for the long delay before the review. Please, consider some initial comments about the patch.

Please, factor out all unrelated changes into separate PRs: say, updating of Travis-CI jobs, Sophia → Vinyl update in the README, fix of the authentification issue (see below). Then squash all relevant commits: nobody is interesting in intermediate changes.

tarantool/const.py

tarantool/mesh_connection.py

unit/suites/test_reconnect.py

tarantool/connection.py

tarantool/mesh_connection.py

unit/suites/test_reconnect.py

tarantool/mesh_connection.py

Totktonada · 2019-04-29T20:47:21Z

BTW, there is the task about distros updating (#123). I have pinged Alexander about that.

Totktonada

Please, rebase and squash changes. Factor out unrelated readme updates (I suggest to file separate PR to don't mix discussions, but don't insist: separate commit is ok too).

I look at the PR more or less briefly, so feel free to discuss certain points here or privately in text or voicely.

tarantool/connection.py

README.rst

tarantool/const.py

tarantool/mesh_connection.py

README.rst

unit/suites/test_mesh.py

tarantool/mesh_connection.py

Totktonada · 2019-06-09T09:20:46Z

I decided to skip review ping-pong and reworked some parts of the patch where I guess I can improve things. I have made the following changes:

Fixed all flake8 warnings in mesh_connection.py and test_mesh.py: trailing whitespaces and long lines.
Prevent a user provided addrs argument from changing: copy it before inserting host+port.
Added warnings for parse_uri() and other cases like a network error during a call to a discovery function.
Removed _opt_reconnect() before a call to a discovery function, but give a warning if something went wrong during the call.
Restrict validation (say, catch ':1234' or 'localhost:65536'), extract it from RoundRobinStrategy; reworked to return warning messages as values to use as warnings or error (see the following bullet).
Raise a configuration error if user provided addresses does not pass validation. Give warnings as before for cluster discovery function result.
Added test cases for wrong cluster discovery function results.
Give a configuration error for empty address list even with connect_now=False. It is needed to keep self.{host,port} consistent with self.strategy.pos and so simplify the code: say, we don't need to explicitly get a next address in connect(), only in _opt_reconnect().
Reverted cycle usage in RoundRobinStrategy to implement self.pos adjusting properly (when a current address is in the new address list).
Reverted self.connected = False in connection.py::close(). I looked at the git history and it is intended to be the internal field. Added test case for connect() after close() in PR test: verify connect() after close() #144. If we want to provide the way to determine whether a connection is alive we need to expose a function. But maybe ping() should be used for this.
Fixed the bug when self.strategy.getnext() gives currently used address: this is why +1 was needed in the loop around super(...)._opt_reconnect() + getnext() in MeshConnection._opt_reconnect().
Re-raise a last error in MeshConnection._opt_reconnect() (before it raises NetworkError class).
Reworked test_mesh.py a bit: added / enhanced comments, cropped extra arguments where it is appropriate, some taste changes.
Expanded the commit message.

Maybe I missed to mention something in the list.

@robinhood23rus Can you please give me feedback about the new version of the patch (force-pushed here).

This feature adds the new optional arguments to the MeshConnection contructor: * `cluster_discovery_function` -- a name of the function which will be periodically called on a currently connected tarantool instance to update a list of MeshConnection addresses. * `cluster_discovery_delay` -- minimal amount of seconds between address list updates (default is 60 seconds). The update of addresses is performed right after successful connecting and before performing a request (if a minimal time passes). This commits changes the round robin retry strategy. Before it performs two attempts to connect to each address reconnect_max_attempts times (3 by default), now it do that only once. The new type of error is added: ConfigurationError. It is risen when a user provides incorrect configuration: say, one of provided addresses is not correct. The new type of warning is added: ClusterDiscoveryWarning. This warning is shown when a something went wrong during cluster discovery: say, one of returned addresses is not correct. Note the difference: a user provided configuration verified strictly, while a cluster discovery function result is filtered (with warnings) and good addresses are applied (if the list is not empty). Aside of the new functionality this commit improves compatibility of MeshConnection API with Connection. The following arguments are added to the MeshConnection constructor: `host`, `port`, `call_16`, `connection_timeout`. An address from `host` / `port` arguments is added to `addrs` (if provided) as the first item. Fixes #134.

Totktonada · 2019-06-10T07:42:51Z

Reorganized parse_uri() to get rid of asserts and make it obvious that its conditions are always met.

denis-ignatenko · 2019-06-10T10:00:25Z

Changes ok. Approve

msiomkin force-pushed the get-node-list-from-nodes branch 3 times, most recently from 7a4659b to 0448226 Compare March 23, 2019 09:52

msiomkin changed the title ~~WIP: Implement getting the cluster nodes list from nodes~~ Implement getting the cluster nodes list from nodes Mar 23, 2019

msiomkin force-pushed the get-node-list-from-nodes branch 4 times, most recently from 9dce8ff to b087be0 Compare March 28, 2019 07:05

Totktonada reviewed Apr 29, 2019

View reviewed changes

denis-ignatenko force-pushed the get-node-list-from-nodes branch from ddcf29c to cf62582 Compare May 23, 2019 16:42

Totktonada reviewed May 28, 2019

View reviewed changes

Totktonada reviewed May 29, 2019

View reviewed changes

unit/suites/test_mesh.py Outdated Show resolved Hide resolved

Totktonada reviewed May 30, 2019

View reviewed changes

tarantool/mesh_connection.py Outdated Show resolved Hide resolved

denis-ignatenko force-pushed the get-node-list-from-nodes branch 2 times, most recently from dca9fd8 to b20057c Compare May 31, 2019 10:15

Totktonada mentioned this pull request May 31, 2019

cluster discovery: don't discard the entire result when there are bad addresses tarantool/tarantool-java#195

Closed

denis-ignatenko force-pushed the get-node-list-from-nodes branch from e1a3395 to 85bc56c Compare June 3, 2019 16:27

denis-ignatenko mentioned this pull request Jun 4, 2019

Hotfix schema version on auth #142

Closed

denis-ignatenko force-pushed the get-node-list-from-nodes branch from 01df289 to c25c651 Compare June 6, 2019 11:30

Totktonada force-pushed the get-node-list-from-nodes branch from c25c651 to 1576d2c Compare June 9, 2019 09:20

Totktonada force-pushed the get-node-list-from-nodes branch from 1576d2c to d64c52b Compare June 10, 2019 07:42

Totktonada merged commit 4dfe5f9 into master Jun 10, 2019

Totktonada deleted the get-node-list-from-nodes branch June 10, 2019 10:19

Implement getting the cluster nodes list from nodes #135

Implement getting the cluster nodes list from nodes #135

Uh oh!

Conversation

msiomkin commented Mar 21, 2019

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada commented Apr 29, 2019

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada commented Jun 9, 2019

Uh oh!

Totktonada commented Jun 10, 2019

Uh oh!

denis-ignatenko commented Jun 10, 2019

Uh oh!

Uh oh!