Implement reconnection strategy class #109
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Totktonada
reviewed
Sep 19, 2018
Totktonada
reviewed
Sep 19, 2018
Totktonada
reviewed
Sep 19, 2018
Totktonada
reviewed
Sep 19, 2018
kbelyavs
force-pushed
the
kbelyavs/gh-106
branch
from
ff0f4af to
46beffb
Compare
Contributor
|
Contributor
|
kbelyavs
force-pushed
the
kbelyavs/gh-106
branch
from
46beffb to
6d62982
Compare
Totktonada
reviewed
Oct 1, 2018
tarantool/__init__.py
Outdated
| ''' | ||
| Create a connection to the mesh of Tarantool servers. | ||
| :param str addrs: A map of server hostname or IP-address and a port. |
Contributor
There was a problem hiding this comment.
The description is a bit unprecise: it is a tuple / a list of maps.
tarantool/mesh_connection.py
Outdated
| reconnect_delay=RECONNECT_DELAY, | ||
| connect_now=True, | ||
| encoding=ENCODING_DEFAULT, | ||
| Strategy=RoundRobinStrategy): |
Contributor
There was a problem hiding this comment.
It is unusual to start a parameter name and / or a class field name from an uppercase letter in Python. From the other side, it is a reference to a class, so I don't sure what is better to do.
testclient.py
Outdated
| @@ -0,0 +1,23 @@ | |||
| #!/usr/bin/env python | |||
Contributor
There was a problem hiding this comment.
I don't think we should add and then remove it.
test/cluster-py/multi.test.py
Outdated
| import time | ||
| import yaml | ||
| from lib.tarantool_server import TarantoolServer | ||
| from tarantool import MeshConnection |
Contributor
There was a problem hiding this comment.
These imports do not work for me.
test/cluster-py/multi.test.py
Outdated
| while True: | ||
| try: | ||
| x = con.ping() | ||
| print 'ping Ok' |
Contributor
There was a problem hiding this comment.
I think it is better to use python-3-style prints, because it is universal.
test/cluster-py/replica.lua
Outdated
| @@ -0,0 +1,13 @@ | |||
| #!/usr/bin/env tarantool | |||
| box_cfg_done = false | |||
test/cluster-py/suite.ini
Outdated
| [default] | ||
| core = tarantool | ||
| script = master.lua | ||
| description = tarantool/box, replication |
Contributor
There was a problem hiding this comment.
Description seems to be not about the suite we have here.
kbelyavs
force-pushed
the
kbelyavs/gh-106
branch
from
6d62982 to
63436b5
Compare
Extend base Connection class to support a list of nodes and an optional Strategy parameter to choose next item from this list. Add built-in reconnect strategy class based on Round-Robin alg. @params: - addrs, list of maps {'host':(HOSTNAME|IP_ADDR), 'port':PORT}. Return next connection or an error if all URIs are unavailable. Closes #106
kbelyavs
force-pushed
the
kbelyavs/gh-106
branch
4 times, most recently
from
e6634a3 to
fb256f1
Compare
Totktonada
reviewed
Oct 10, 2018
Contributor
Totktonada
left a comment
Totktonada
left a comment
There was a problem hiding this comment.
Comments re testing are below.
I think we can setup CI with make install for now (and merge the PR) and file a follow up issue to use the connector from the repository during make test / cd test && ./test-run.py.
test/cluster-py/multi.test.py
Outdated
| host, port = addr.split(':') | ||
| addrs.append({'host': host, 'port': int(port)}) | ||
|
|
||
| thread = Thread(target=check_cluster, args=(addrs, )) |
Contributor
There was a problem hiding this comment.
I think it is better to create purely sequencial test and don't lean on timings. Like so (schematically):
setup instances
setup mesh connection
perform select
stop instance 1
perform select
stop instance 2
start instance 1
perform select
stop instance 1
perform select
fb256f1 to
ca7dc5b
Compare
Totktonada
reviewed
Oct 14, 2018
test/cluster-py/replica.lua
Outdated
| local SOCKET_DIR = require('fio').cwd() | ||
|
|
||
| local function instance_uri(instance_id) | ||
| return SOCKET_DIR..'/autobootstrap'..instance_id..'.sock'; |
Contributor
There was a problem hiding this comment.
Why replica? Why autobootstrap? Why multi?
I think we should adjust naming.
Totktonada
reviewed
Oct 14, 2018
Contributor
|
I'll ok after:
Also I think we can use default server as the first instance in the test (and setup one more). Please, proceed with that and merge. I don't think I should look into that again. |
Totktonada
approved these changes
Oct 14, 2018
kbelyavs
force-pushed
the
kbelyavs/gh-106
branch
from
ca7dc5b to
436218d
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend base Connection class to support a list of nodes and an
optional Strategy parameter to choose next item from this list.
Add built-in reconnect strategy class based on Round-Robin alg.
@params:
Return next connection or an error if all URIs are unavailable.
Closes #106