Re: [Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

>>> @TarantoolBot document
>>>      Title: Add session_setting
>>
>> The doc request is corrupted, docbot won't understand that because
>> of leading whitespaces.
>>
>>> iproto_error_format setting has been added to _session_settings
>>
>> Looks like there is a lack of global setting similar to what we had
>> for tracebacks. Currently, when the option is false (by default), and
>> I want to use the new format everywhere, I need to find every single
>> place where I create a new session, and put there code saying
>>
>>      box.session.settings.error_format = new/old/whatever
>>
>> I think there should be a global option when a user's application is
>> ready to switch to the new format completely. Otherwise it is going
>> to be hell to find all places where a new session is created, and patch
>> them.
>>
>> Just a reminder - every fiber.new(), fiber.create() creates a session,
>> every iproto connection is a session.
> This was discussed in TPT chat with Mons and Nazarov, and after that with you, Turenko, Mons ... in zoom. I was orienteted by the net.box session and I might not know something. Where do you suggest storing the setting?

Looks like you didn't understood me. I don't propose to remove the
session local option. I propose to provide an ability to override it
with a global option if necessary. So we have session option by
default with the old format, and global option unset.

If someone is ready to start using new errors everywhere, he just turns
the global option on.

If someone is not ready, he continues using session local option.

Why that may be needed - I said above. Turning that option on for every
session is going to be very tricky. But maybe it can be moved to the next
release (the global option). Don't know. The issue is already a huge
blackhole which absorbed lots of independent initially unplanned
features. The more we move for later, the better.

You should try to ask users in the red chat. It helps sometimes, they
can share their opinions and experience of upgrades. Just formulate your
question in a short but detailed manner, provide a couple of
ready-to-choose answers, probably a poll.

>>> +        local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 1)
>>
>> 3. This won't work in case it is an instance bootstrapped from the master
>> branch before 2.4.1 was released. I don't know how to fix it now.
> Sorry, I think this should not work until the release.

Exactly for 'between-releases' users we introduced the new versioning schema
with 4 numbers. I don't think we can just say fuck off to them now. I
would better drop this from the patchset and added when somebody explicitly
asks for that, with good design and planning.

>>
>>> +        if not ext_err_supported then
>>> +            box.error(box.error.PROC_LUA,
>>> +                      "Server doesn't support extended error format")
>>> +        end
>>> +        remote.space._session_settings:update('iproto_error_format',
>>> +                                              {{'=', 2, 1}})
>>
>> 4. This is additional network hop. I don't think it should be done
>> automatically. I would let users do that.
> It's network hop only if option will be set.As optimization it can be included to auth packet in future.

Auth packets have nothing to do with session settings. It would be
encapsulation violation. Moreover, in addition to settings we may
want to configure more things in future. What if an encryption will
be added to Tarantool? What if we will add key exchange? I think we
should not add a new option + 1 network hop for every 'negotiation'
thing. Better omit this for now and design this properly when we have
more time.

I don't propose to remove your code. I just propose to finish some
non-critical parts of it later.