From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 9C3A74696C3 for ; Thu, 16 Apr 2020 12:36:28 +0300 (MSK) References: <4d6b0c27784a8abe7573baa3c859a3ebca07f1fb.1586505741.git.lvasiliev@tarantool.org> <5c3af252-f1ce-3e93-ac64-135eed764e06@tarantool.org> From: lvasiliev Message-ID: Date: Thu, 16 Apr 2020 12:36:27 +0300 MIME-Version: 1.0 In-Reply-To: <5c3af252-f1ce-3e93-ac64-135eed764e06@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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