From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 B31DF4696C3 for ; Thu, 16 Apr 2020 03:06:09 +0300 (MSK) References: <4d6b0c27784a8abe7573baa3c859a3ebca07f1fb.1586505741.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5c3af252-f1ce-3e93-ac64-135eed764e06@tarantool.org> Date: Thu, 16 Apr 2020 02:06:07 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" 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: lvasiliev Cc: tarantool-patches@dev.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.