Re: [Tarantool-patches] [PATCH v4 net.box] Add predefined system events for pub/sub

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch, good job!

See some comments below.

On 22.02.2022 13:56, Yan Shtunder wrote:
> Added predefined system events: box.status, box.id, box.election and
> box.schema.
> 
> Closes #6260
> 
> @TarantoolBot document
> Title: Built-in events for pub/sub
> 
> Built-in events are needed, first of all, in order to learn who is the
> master, unless it is defined in an application specific way. Knowing who
> is the master is necessary to send changes to a correct instance, and
> probably make reads of the most actual data if it is important. Also
> defined more built-in events for other mutable properties like replication
> list of the server, schema version change and instance state.

1. There is no an event about replication list of the server.
Also you need to provide more details. Which events are introduced? You
didn't give names. Which fields each of them has?

There are names in the example below, but it does not reveal the events'
content.

> Example usage:
> 
>  * Client:
>    ```lua
>    conn = net.box.connect(URI)
>    -- Subscribe to updates of key 'box.id'
>    w = conn:watch('box.id', function(key, value)
>        assert(key == 'box.id')
>        -- do something with value
>    end)
>    -- or to updates of key 'box.status'
>    w = conn:watch('box.status', function(key, value)
>        assert(key == 'box.status')
>        -- do something with value
>    end)
>    -- or to updates of key 'box.election'
>    w = conn:watch('box.election', function(key, value)
>        assert(key == 'box.election')
>        -- do something with value
>    end)
>    -- or to updates of key 'box.schema'
>    w = conn:watch('box.schema', function(key, value)
>        assert(key == 'box.schema')
>        -- do something with value
>    end)
>    -- Unregister the watcher when it's no longer needed.
>    w:unregister()
>    ```
> ---
> Issue: https://github.com/tarantool/tarantool/issues/6260
> Patch: https://github.com/tarantool/tarantool/tree/yshtunder/gh-6260-events-v4
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index b85d279e3..cd48cd6aa 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -57,6 +57,14 @@
>  #include "sequence.h"
>  #include "sql.h"
>  #include "constraint_id.h"
> +#include "box.h"
> +
> +static void
> +box_schema_version_bump(void)
> +{
> +	++schema_version;
> +	box_broadcast_schema();
> +}

2. Please, move it below the comment on the next line. To keep
the functions grouped.

>  /* {{{ Auxiliary functions and methods. */
> 
> diff --git a/test/box-luatest/gh_6260_add_builtin_events_test.lua b/test/box-luatest/gh_6260_add_builtin_events_test.lua
> new file mode 100644
> index 000000000..c11d79e30
> --- /dev/null
> +++ b/test/box-luatest/gh_6260_add_builtin_events_test.lua
> @@ -0,0 +1,294 @@
> +local t = require('luatest')
> +local net = require('net.box')
> +local cluster = require('test.luatest_helpers.cluster')
> +local server = require('test.luatest_helpers.server')
> +local fiber = require('fiber')
> +
> +local g = t.group('gh_6260')
> +
> +g.before_test('test_box_status', function(cg)
> +    cg.cluster = cluster:new({})
> +    cg.master = cg.cluster:build_server({alias = 'master'})
> +    cg.cluster:add_server(cg.master)
> +    cg.cluster:start()
> +end)
> +
> +g.after_test('test_box_status', function(cg)
> +    cg.cluster.servers = nil
> +    cg.cluster:drop()
> +end)
> +
> +g.test_box_status = function(cg)
> +    local c = net.connect(cg.master.net_box_uri)
> +
> +    local result = {}
> +    local result_no = 0
> +    local watcher = c:watch('box.status',
> +                            function(name, state)
> +                                assert(name == 'box.status')
> +                                result = state
> +                                result_no = result_no + 1
> +                            end)
> +
> +    -- initial state should arrive
> +    t.helpers.retrying({}, function()
> +        while result_no < 1 do fiber.sleep(0.000001) end
> +    end)

3. With retrying() you are not supposed to have 'while' loop. It is done
inside of the function for a limited time. The problem with custom
'while' loops is that people usually do not add any timeout and do not
handle exceptions. Please, take a look again at other usages of retrying()
for a reference. The same in the similar places below.

> +
> +    t.assert_equals(result,
> +                    {is_ro = false, is_ro_cfg = false, status = 'running'})
> +
> +    -- test orphan status appearance
> +    cg.master:exec(function(repl)
> +        box.cfg{
> +            replication = repl,
> +			replication_connect_timeout = 0.001,

4. Indentation is broken.

> +            replication_timeout = 0.001,
> +        }
> +    end, {{server.build_instance_uri('master'), server.build_instance_uri('replica')}})

5. You are out of 80 symbols.

> +    -- here we have 2 notifications: entering ro when can't connect
> +    -- to master and the second one when going orphan
> +    t.helpers.retrying({}, function()
> +        while result_no < 3 do fiber.sleep(0.000001) end
> +    end)
> +    t.assert_equals(result,
> +                    {is_ro = true, is_ro_cfg = false, status = 'orphan'})

<...>

> +g.test_box_election = function(cg)
> +    local c = {}
> +    c[1] = net.connect(cg.instance_1.net_box_uri)
> +    c[2] = net.connect(cg.instance_2.net_box_uri)
> +    c[3] = net.connect(cg.instance_3.net_box_uri)
> +
> +    local res = {}
> +    local res_n = {0, 0, 0}
> +
> +    for i = 1, 3 do
> +        c[i]:watch('box.election',
> +                   function(n, s)
> +                       t.assert_equals(n, 'box.election')
> +                       res[i] = s
> +                       res_n[i] = res_n[i] + 1
> +                   end)
> +    end
> +    t.helpers.retrying({}, function()
> +        while res_n[1] + res_n[2] + res_n[3] < 3 do fiber.sleep(0.00001) end
> +    end)
> +
> +    -- verify all instances are in the same state
> +    t.assert_equals(res[1], res[2])
> +    t.assert_equals(res[1], res[3])
> +
> +    -- wait for elections to complete, verify leader is the instance_1
> +    -- trying to avoid the exact number of term - it can vary

6. You just said "trying to avoid the exact number of term" and used an
exact number of term a few lines below. Lets better not rely on an
exact term number. It is enough to test that it exists and is the same in
all 3 events.

t.assert_covers() allows to omit certain fields. Because it is not 'equal',
it is 'covers'. A subset of fields is enough. The others can be checked in
a special way. Like term.

> +    local instance1_id = cg.instance_1:instance_id()
> +
> +    cg.instance_1:exec(function() box.cfg{election_mode='candidate'} end)
> +    cg.instance_2:exec(function() box.cfg{election_mode='voter'} end)
> +    cg.instance_3:exec(function() box.cfg{election_mode='voter'} end)
> +
> +    cg.instance_1:wait_election_leader_found()
> +    cg.instance_2:wait_election_leader_found()
> +    cg.instance_3:wait_election_leader_found()
> +
> +    t.assert_covers(res,
> +        {
> +         {leader = instance1_id, is_ro = false, role = 'leader', term = 2},
> +         {leader = instance1_id, is_ro = true, role = 'follower', term = 2},
> +         {leader = instance1_id, is_ro = true, role = 'follower', term = 2}
> +        })

<...>

> Hi! Thank you for the review!

Lets next time put the comment responses before the new patch. I noticed
this part of the email only when finished the new review.