From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 30BCD46970E for ; Sat, 28 Dec 2019 13:57:08 +0300 (MSK) Date: Sat, 28 Dec 2019 13:57:07 +0300 From: Alexander Turenko Message-ID: <20191228105707.uieh5lnabhqrsu62@tkn_work_nb> References: <4f09686e246ae3507121bb66e07cbe1e5c94c608.1577449923.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4f09686e246ae3507121bb66e07cbe1e5c94c608.1577449923.git.imeevma@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 1/1] sql: add another way to change SQL default engine List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org I don't mind in general, but please consider several notes below. WBR, Alexander Turenko. On Fri, Dec 27, 2019 at 03:33:12PM +0300, imeevma@tarantool.org wrote: > sql: add another way to change SQL default engine > > Due to the removal of the sql_default_engine pragma in 2.3.1, this > patch adds another way to change this setting. Nit: I would say 'implements' rather then 'adds', because the letter can be read as something configurable by a user. I would mention that it affects only *.test.sql tests, just in case. I think it would be good to link the issue about it here. > --- > https://github.com/tarantool/test-run/tree/imeevma/fix_sql_default_engine > > lib/tarantool_server.py | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py > index 6f71b5b..20a398e 100644 > --- a/lib/tarantool_server.py > +++ b/lib/tarantool_server.py > @@ -145,12 +145,19 @@ class LuaTest(Test): > return True > > engine = self.run_params['engine'] > - command = "pragma sql_default_engine='{}'".format(engine) > - result = self.send_command(command, ts, 'sql') > - result = result.replace('\r\n', '\n') > - if result != '---\n- row_count: 0\n...\n': > - sys.stdout.write(result) > - return False > + > + command_new = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " + \ > + "WHERE \"name\" = 'sql_default_engine'").format(engine) > + result_new = self.send_command(command_new, ts, 'sql') > + result_new = result_new.replace('\r\n', '\n') > + > + command_old = "pragma sql_default_engine='{}'".format(engine) > + result_old = self.send_command(command_old, ts, 'sql') > + result_old = result_old.replace('\r\n', '\n') > + if result_new != '---\n- row_count: 1\n...\n': > + if result_old != '---\n- row_count: 0\n...\n': > + sys.stdout.write(result_old) > + return False I would make it like so: 1. Send "pragma ...", await a result. - If succeeds, return True. - Otherwise proceed with the step 2. 2. Send "UPDATE ...", await a result. - If succeeds, return True. - Otherwise return False. This way we'll save one extra hop on a fresh tarantool. Not so big overhead for an action that is run once for a test, but we can overcome it and pay nothing, so it looks worthful for me. Please, look into CI: it reports a flake8 error. You can use `make lint` locally to verify python code style. Please, double-check that everything works as expected for both tarantool versions (with pragma support and with _session_settings): let's run a .test.sql test and verify that it creates spaces in an expected engine. It should be easier using `\set language lua` command: it is simple to assess a space engine and compare it with test_run:get_cfg('engine'). README states that test-run use "pragma <...>" to set an engine. It would be good to mention the new way together with the old one. > > return True > > -- > 2.7.4 BTW, the subject of the email is the following: | [PATCH 1/1] sql: add another way to change SQL default engine I suggest to add a repo prefix when it is not tarantool itself. Look as Vlad does it: | [PATCH vshard 1/1] travis-ci: deploy packages from tagged revisions https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/001304.html