Implement reconnection strategy class by kbelyavs · Pull Request #109 · tarantool/tarantool-python

kbelyavs · 2018-09-19T17:36:20Z

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:

peers

Return next connection or an error if all URIs are unavailable.

Closes #106

Totktonada · 2018-10-01T13:18:39Z

$ flake8 tarantool/__init__.py tarantool/mesh_connection.py 
tarantool/__init__.py:53:1: E302 expected 2 blank lines, found 1
tarantool/mesh_connection.py:16:1: E302 expected 2 blank lines, found 1
tarantool/mesh_connection.py:26:1: E302 expected 2 blank lines, found 1
tarantool/mesh_connection.py:38:9: F841 local variable 'addr' is assigned to but never used
tarantool/mesh_connection.py:45:33: E128 continuation line under-indented for visual indent
tarantool/mesh_connection.py:46:33: E128 continuation line under-indented for visual indent
tarantool/mesh_connection.py:47:33: E128 continuation line under-indented for visual indent

Totktonada · 2018-10-01T15:25:09Z

$ flake8 test/cluster-py/*.py
test/cluster-py/multi.test.py:18:1: E302 expected 2 blank lines, found 1
test/cluster-py/multi.test.py:30:27: E999 SyntaxError: invalid syntax
test/cluster-py/multi.test.py:41:1: E305 expected 2 blank lines after class or function definition, found 1
test/cluster-py/multi.test.py:52:12: E201 whitespace after '['
test/cluster-py/multi.test.py:52:19: E202 whitespace before ']'
test/cluster-py/multi.test.py:66:67: E251 unexpected spaces around keyword / parameter equals
test/cluster-py/multi.test.py:66:69: E251 unexpected spaces around keyword / parameter equals
test/cluster-py/multi.test.py:100:23: E251 unexpected spaces around keyword / parameter equals
test/cluster-py/multi.test.py:100:25: E251 unexpected spaces around keyword / parameter equals
test/cluster-py/multi.test.py:100:45: E251 unexpected spaces around keyword / parameter equals
test/cluster-py/multi.test.py:100:47: E251 unexpected spaces around keyword / parameter equals

Totktonada · 2018-10-01T13:20:39Z

+    '''
+    Create a connection to the mesh of Tarantool servers.
+
+    :param str addrs: A map of server hostname or IP-address and a port.


The description is a bit unprecise: it is a tuple / a list of maps.

Totktonada · 2018-10-01T13:25:31Z

+                 reconnect_delay=RECONNECT_DELAY,
+                 connect_now=True,
+                 encoding=ENCODING_DEFAULT,
+                 Strategy=RoundRobinStrategy):


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.

Totktonada · 2018-10-01T14:37:34Z

@@ -0,0 +1,23 @@
+#!/usr/bin/env python


I don't think we should add and then remove it.

Totktonada · 2018-10-01T15:24:36Z

+import time
+import yaml
+from lib.tarantool_server import TarantoolServer
+from tarantool import MeshConnection


These imports do not work for me.

Totktonada · 2018-10-01T15:31:24Z

+    while True:
+        try:
+            x = con.ping()
+            print 'ping Ok'


I think it is better to use python-3-style prints, because it is universal.

Totktonada · 2018-10-01T16:07:37Z

@@ -0,0 +1,13 @@
+#!/usr/bin/env tarantool
+box_cfg_done = false


Is not used.

Totktonada · 2018-10-01T16:08:22Z

+[default]
+core = tarantool
+script =  master.lua
+description = tarantool/box, replication


Description seems to be not about the suite we have here.

@params

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

Totktonada

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.

Totktonada · 2018-10-10T01:00:55Z

+    host, port = addr.split(':')
+    addrs.append({'host': host, 'port': int(port)})
+
+thread = Thread(target=check_cluster, args=(addrs, ))


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

Totktonada · 2018-10-14T06:34:56Z

+local SOCKET_DIR = require('fio').cwd()
+
+local function instance_uri(instance_id)
+    return SOCKET_DIR..'/autobootstrap'..instance_id..'.sock';


Why replica? Why autobootstrap? Why multi?

I think we should adjust naming.

Totktonada · 2018-10-14T06:39:46Z

I'll ok after:

Handle CI somehow (issue at least).
Use appropriate naming in the test (replica, multi, autobootstrap → ...).

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.

kbelyavs requested a review from Totktonada September 19, 2018 17:36

Totktonada reviewed Sep 19, 2018

View reviewed changes

Comment thread client.py Resolved

Totktonada reviewed Sep 19, 2018

View reviewed changes

Comment thread client.py Resolved

Totktonada reviewed Sep 19, 2018

View reviewed changes

Comment thread client.py Resolved

Totktonada reviewed Sep 19, 2018

View reviewed changes

Comment thread client.py Resolved

kbelyavs force-pushed the kbelyavs/gh-106 branch from ff0f4af to 46beffb Compare September 25, 2018 14:30

kbelyavs force-pushed the kbelyavs/gh-106 branch from 46beffb to 6d62982 Compare October 1, 2018 16:08

Totktonada reviewed Oct 1, 2018

View reviewed changes

kbelyavs force-pushed the kbelyavs/gh-106 branch from 6d62982 to 63436b5 Compare October 2, 2018 08:56

kbelyavs force-pushed the kbelyavs/gh-106 branch 4 times, most recently from e6634a3 to fb256f1 Compare October 8, 2018 10:23

Totktonada reviewed Oct 10, 2018

View reviewed changes

kbelyavs force-pushed the kbelyavs/gh-106 branch from fb256f1 to ca7dc5b Compare October 12, 2018 09:55

Totktonada reviewed Oct 14, 2018

View reviewed changes

Comment thread test/cluster-py/multi.test.py Resolved

Totktonada approved these changes Oct 14, 2018

View reviewed changes

Konstantin Belyavskiy added 2 commits October 16, 2018 17:10

Add test-run and a test

ea9d082

move unit tests to unit

436218d

kbelyavs force-pushed the kbelyavs/gh-106 branch from ca7dc5b to 436218d Compare October 16, 2018 14:12

kbelyavs merged commit 14f52b5 into master Oct 16, 2018

Totktonada mentioned this pull request Oct 18, 2018

update tarantool-python connector tarantool/test-run#126

Closed

Totktonada deleted the kbelyavs/gh-106 branch February 19, 2019 10:52

Totktonada mentioned this pull request Feb 10, 2021

bump test-run version #188

Merged

		@@ -0,0 +1,13 @@
		#!/usr/bin/env tarantool
		box_cfg_done = false

Conversation

kbelyavs commented Sep 19, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada commented Oct 1, 2018

Uh oh!

Totktonada commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Totktonada commented Oct 14, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants