From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: "v.shpilevoy@tarantool.org" Message-Id: <9C4A63BA-42EB-4EB6-A19C-39102FED2F0F@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_6B755E31-8D6B-4ED6-BE5C-663F811CA1BD" Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Date: Thu, 22 Mar 2018 22:50:04 +0300 In-Reply-To: <20180322193312.GA18129@atlas> References: <10c500ef97902f645111fd2d46e756bf8dfdac1d.1521745741.git.v.shpilevoy@tarantool.org> <20180322193312.GA18129@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org, vdavydov.dev@gmail.com List-ID: --Apple-Mail=_6B755E31-8D6B-4ED6-BE5C-663F811CA1BD Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 22 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 22:33, = Konstantin Osipov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0): >=20 > * Vladislav Shpilevoy > [18/03/22 22:18]: >=20 > The patch generally looks good to me, I can only nitpick on new > things.=20 >=20 > How about renaming do_connect to connect_impl and wrap_connect to > wrap_socket or simply wrap, or maybe bless or imbue? Ok. >=20 >> + if existing_connection then >> + connection =3D existing_connection >> + existing_connection =3D nil >=20 > Why is it necessary to set it to nil? On reconnect it will call protocol_sm again, and it must no get old = existing_connection again. >=20 >> + assert(greeting) >> + assert(greeting.protocol ~=3D 'Lua console') >=20 > Please keep in mind that in Lua there are no asserts - it's a > runtime check. Ok. >> + >> +local function wrap_socket(connection, greeting, url, opts) >> + if connection =3D=3D nil or type(greeting) ~=3D 'table' then >> + error('Usage: netbox.wrap_socket(socket, greeting, [opts])') >> + end >> + if greeting.protocol =3D=3D 'Lua console' then >> + error('Can not wrap console socket') >> + end >> + opts =3D opts or {} >> + if not opts.user and not opts.password then >> + opts.user, opts.password =3D url.login, url.password >> + end >> + return do_connect(url.host, url.service, opts, connection, = greeting) >> +end >=20 > Looks like you should make the next step, i.e. take the piece of > do_connect which establishes a connection and move it to > connect(), and then you can the remains of do_connect into wrap() > and call wrap() connect(). >=20 > Did you try to do it? Had any trouble? Connection establishment must be in connect inside create_transport, = because even if an existing socket was wrapped, it still can have = opts.reconnect_after option and the following scenario: socket =3D ... con =3D netbox.wrap_socket(socket, ..., {reconnect_after =3D ...}) -- -- use connection ... -- Suppose it is broken due to error. Then netbox will try to establish a = new connection after 'reconnect_after' seconds. >=20 > --=20 > Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 > http://tarantool.org - = www.twitter.com/kostja_osipov --Apple-Mail=_6B755E31-8D6B-4ED6-BE5C-663F811CA1BD Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

22 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 22:33, = Konstantin Osipov <kostja@tarantool.org> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0):

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/03/22 22:18]:

The patch generally looks good to me, I = can only nitpick on new
things.

How about renaming do_connect to = connect_impl and wrap_connect to
wrap_socket or = simply wrap, or maybe bless or imbue?

Ok.

+ =        if existing_connection then
+ =            connecti= on =3D existing_connection
+ =            existing= _connection =3D nil

Why is it necessary = to set it to nil?

On = reconnect it will call protocol_sm again, and it must no get old = existing_connection again.

+ = assert(g= reeting)
+ = assert(g= reeting.protocol ~=3D 'Lua console')

Please keep in mind that in Lua there are no = asserts - it's a
runtime = check.

Ok.

+
+local function wrap_socket(connection, = greeting, url, opts)
+    if connection =3D=3D= nil or type(greeting) ~=3D 'table' then
+ =        error('Usage: = netbox.wrap_socket(socket, greeting, [opts])')
+ =    end
+    if = greeting.protocol =3D=3D 'Lua console' then
+ =        error('Can not wrap console = socket')
+    end
+ =    opts =3D opts or {}
+ =    if not opts.user and not opts.password then
+        opts.user, = opts.password =3D url.login, url.password
+ =    end
+    return = do_connect(url.host, url.service, opts, connection, greeting)
+end

Looks like you = should make the next step, i.e. take the piece of
do_connect which establishes a connection and = move it to
connect(), and then you can the remains = of do_connect into wrap()
and call wrap() = connect().

Did you try to do = it? Had any trouble?

Connection = establishment must be in connect inside create_transport, because even = if an existing socket was wrapped, it still can have = opts.reconnect_after option and the following scenario:

socket = =3D ...

con =3D netbox.wrap_socket(socket, ..., = {reconnect_after =3D ...})

-- use connection = ...

Suppose it is = broken due to error. Then netbox will try to establish a new connection = after 'reconnect_after' seconds.

--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 = 32
http://tarantool.org - www.twitter.com/kostja_osipov

= --Apple-Mail=_6B755E31-8D6B-4ED6-BE5C-663F811CA1BD--