* Re: [Tarantool-patches] [PATCH 1/1] sql: add another way to change SQL default engine
2019-12-27 12:33 [Tarantool-patches] [PATCH 1/1] sql: add another way to change SQL default engine imeevma
@ 2019-12-28 10:57 ` Alexander Turenko
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Turenko @ 2019-12-28 10:57 UTC (permalink / raw)
To: Mergen Imeev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 2+ messages in thread