Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings

[Chris Sosnin <k.sosnin@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]



> On 17 Mar 2020, at 17:27, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 16 Mar 23:53, Vladislav Shpilevoy wrote:
>> On 16/03/2020 17:08, Nikita Pettik wrote:
>>> On 17 Feb 15:12, Chris Sosnin wrote:
>>>> - space_object:update() is hard to use for configuring session settings,
>>>> so we provide box.session.setting table, which can be used in a much more
>>>> native way.
>>>> 
>>>> - Prior to this patch sql settings were not accessible before box.cfg()
>>>> call, even though these flags can be set right after session creation.
>>>> 
>>>> Part of #4711
>>>> ---
>>> 
>>> tarantool> box.session.settings.sql_vdbe_debug
>>> ---
>>> - false
>>> ...
>>> 
>>> tarantool> box.session.settings.sql_vdbe_debug = true
>>> ---
>>> ...
>>> 
>>> tarantool> box.session.settings.sql_vdbe_debug
>>> ---
>>> - true
>>> ...
>> 
>> Yeah, we can ban this. To avoid confusion.
>> 
>>> 
>>> tarantool> box.execute("select 1")
>>> ---
>>> - metadata:
>>>  - name: '1'
>>>    type: integer
>>>  rows:
>>>  - [1]
>>> ...
>>> 
>>> Looks inconsistent. Can we use instead of :set() method simple
>>> table value assignment? Otherwise accessing row table values
>> 
>> Assignment would require not to store settings in box.session.settings,
>> to be able to redefine __newindex metamethod. If we don't store them, we
>> kill autocompletion, which was asked explicitly by somebody.
>> 
>> But I am on your side here - I don't think autocompletion worth this
>> complication. Who wants to look at existing settings can just print
>> box.sessions.settings table.
>> 
>>> should be disallowed. Same concerns :get() method. Why ever
>>> anyone should bother with :get() when one can access table value
>>> via simple indexing?
>> 
>> Hm. But there is no :get() method. We didn't implement getting, because
>> no one asked for this.
> 
> As far as I remember no one either asked for :set() method (except for me) :)

This method is a workaround to allow console autocompletion, Lua doesn’t allow you to overload
indexing for the keys already present in the table. But here I agree that this implementation is rather
misleading. I will rework the patch to allow settings.<name> = <value> syntax.

> 
>> And indeed, usually you just set settings, without
>> checking if set really worked. I was thinking we could introduce get if
>> someone asks for that. But up to you. Can be added now as well.
> 
> I do not insist since do not consider this to be prio1 task (as well
> as any other issue connected with settings machinery).


[-- Attachment #2: Type: text/html, Size: 8115 bytes --]