Re: [tarantool-patches] Re: [PATCH] replication: automatic skip duplicating rows in replication

["Konstantin Belyavskiy" <k.belyavskiy@tarantool.org>]

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

Please take a look at newer version.

>Пятница, 13 апреля 2018, 11:24 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:
>
>On Thu, Apr 12, 2018 at 02:01:25PM +0300, Konstantin Belyavskiy wrote:
>> ticket:  https://github.com/tarantool/tarantool/issues/3270
>> branch:  https://github.com/tarantool/tarantool/compare/gh-3270-add-skip-conflict-row-option
>> 
>> In case of attempting to insert a duplicate key, an error ER_TUPLE_FOUND
>> occured, which led to disconnect. 
>> Introduce new oftion: 'silent_skip_conflict_rows', if set, then error of
>> this type will be ignored.
>> 
>> Closes #3270
>> ---
>>  src/box/applier.cc                          |  15 +++-
>>  src/box/lua/load_cfg.lua                    |   3 +
>>  test/app-tap/init_script.result             |  39 +++++----
>>  test/box/admin.result                       |   2 +
>>  test/box/cfg.result                         |   4 +
>>  test/replication/replica_skip_row.lua       |  10 +++
>>  test/replication/skip_conflict_row.result   | 129 ++++++++++++++++++++++++++++
>>  test/replication/skip_conflict_row.test.lua |  46 ++++++++++
>>  8 files changed, 228 insertions(+), 20 deletions(-)
>>  create mode 100644 test/replication/replica_skip_row.lua
>>  create mode 100644 test/replication/skip_conflict_row.result
>>  create mode 100644 test/replication/skip_conflict_row.test.lua
>> 
>> diff --git a/src/box/applier.cc b/src/box/applier.cc
>> index 9aa951c34..5ebe67e27 100644
>> --- a/src/box/applier.cc
>> +++ b/src/box/applier.cc
>> @@ -47,6 +47,7 @@
>>  #include "xrow_io.h"
>>  #include "error.h"
>>  #include "session.h"
>> +#include "cfg.h"
>> 
>>  STRS(applier_state, applier_STATE);
>> 
>> @@ -505,7 +506,19 @@ applier_subscribe(struct applier *applier)
>>  			 */
>>  			vclock_follow(&replicaset.vclock, row.replica_id,
>>  				      row.lsn);
>> -			xstream_write_xc(applier->subscribe_stream, &row);
>> +			if (xstream_write(applier->subscribe_stream, &row) != 0) {
>> +				struct error *e = diag_last_error(diag_get());
>> +				/**
>> +				 * Silently skip ER_TUPLE_FOUND error if such
>> +				 * option is set in config.
>> +				 */
>> +				if (e->type == &type_ClientError &&
>> +				    box_error_code(e) == ER_TUPLE_FOUND &&
>
>> +				    cfg_geti("silent_skip_conflict_rows"))
>
>cfg_geti() is a heavy operation. I think you should cache the value of
>this configuration option in C. 
I hope conflicts will occurs rather rare, over-wise something goes wrong..
So it's not a big overkill.
>
>> +					diag_clear(diag_get());
>> +				else
>> +					diag_raise();
>> +			}
>
>What about UPDATE over a non-existent key or vinyl transaction conflict?
>Those can also happen due to replication conflicts. May be, we should
>ignore all errors of type ClientError?
No, in this ticket I want to solve a certain problem, let's consider other cases in separate ticket.
>
>>  		}
>>  		if (applier->state == APPLIER_SYNC ||
>>  		    applier->state == APPLIER_FOLLOW)
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>> index 89fd7745e..372f00f25 100644
>> --- a/src/box/lua/load_cfg.lua
>> +++ b/src/box/lua/load_cfg.lua
>> @@ -62,6 +62,7 @@ local default_cfg = {
>>      feedback_enabled      = true,
>>      feedback_host         = " https://feedback.tarantool.io ",
>>      feedback_interval     = 3600,
>> +    silent_skip_conflict_rows = false,
>
>All replication-related options should have replication_ prefix.
>What about replication_ignore_conflicts? 
Replace with "replication_skip_conflict" (from ticket Name)
>> diff --git a/test/replication/skip_conflict_row.test.lua b/test/replication/skip_conflict_row.test.lua
>> new file mode 100644
>> index 000000000..5cfbff339
>> --- /dev/null
>> +++ b/test/replication/skip_conflict_row.test.lua
>> @@ -0,0 +1,46 @@
>> +env = require('test_run')
>> +test_run = env.new()
>> +engine = test_run:get_cfg('engine')
>> +
>> +box.schema.user.grant('guest', 'read,write,execute', 'universe')
>> +box.schema.user.grant('guest', 'replication')
>> +
>> +space = box.schema.space.create('test', {engine = engine});
>> +index = box.space.test:create_index('primary')
>> +
>> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica_skip_row.lua'")
>
>AFAIU you don't need to add a new script - you can reuse replica.lua and
>set the new configuration option after bootstrap. 
Yes, you're right, updated and remove new replica_skip_row.lua
>
>> +test_run:cmd("start server replica")
>> +
>> +repl = box.cfg.replication
>> +box.cfg{replication = ""}
>> +box.info.status
>
>I don't understand why you turn off replication here. 
Fixed, you are right, it's not necessary, since it's not a mesh.
>
>> +
>> +test_run:cmd("switch replica")
>> +repl = box.cfg.replication
>> +box.cfg{replication = ""}
>
>... and here 
Removed
>
>> +box.space.test:insert{2}
>> +box.space.test:insert{1}
>> +
>> +test_run:cmd("switch default")
>> +space:insert{1}
>> +space:select{}
>> +box.cfg{replication = repl}
>> +box.info.status
>> +
>> +test_run:cmd("switch replica")
>> +box.cfg{replication = repl}
>> +require('fiber').sleep(0.01)
>> +box.info.replication[1].upstream.message
>> +box.info.replication[1].upstream.status
>> +box.space.test:select{}
>> +
>> +test_run:cmd("switch default")
>> +space:select{}
>> +box.info.status
>> +
>> +-- cleanup
>> +test_run:cmd("stop server replica")
>> +test_run:cmd("cleanup server replica")
>> +box.space.test:drop()
>> +box.schema.user.revoke('guest', 'replication')
>> +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>


Best regards,
Konstantin Belyavskiy
k.belyavskiy@tarantool.org

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