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

[lvasiliev]

Hi! Thanks you for the feedback.

On 16.04.2020 3:06, Vladislav Shpilevoy wrote:
> 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.
> 
Ok. But for beginning, I need to understand where this flag can be
located. I will think about that but if you have a proposal, please,
write it.
>>>> +        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.
> 
Ok.I have remove it, but:"Don't have 'between-releases' users a version 
more when previous release?"
>>>
>>>> +        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.
> 
ok