[Tarantool-patches] [PATCH v5 4/4] box: start counting local space requests separately
Serge Petrenko
sergepetrenko at tarantool.org
Tue Apr 7 18:48:56 MSK 2020
> 4 апр. 2020 г., в 23:51, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> написал(а):
>
> Thanks for the patch!
Thanks for the review!
>
> See 3 comments below.
>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index bf95d1b5e..0762266b0 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1879,6 +1879,23 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>> say_info("remote vclock %s local vclock %s",
>> vclock_to_string(&replica_clock), vclock_to_string(&vclock));
>>
>> + /*
>> + * Replica clock is used in gc state and recovery
>> + * initialization, so we need to replace the remote 0-th
>> + * component with our own one. This doesn't break
>> + * recovery: it finds the WAL with a vclock strictly less
>> + * than replia clock in all components except the 0th one.
>> + * This leads to finding the correct WAL, if it exists,
>> + * since we do not need to recover local rows (the ones,
>> + * that contribute to the 0-th vclock component).
>> + * Note, that it would be bad to set 0-th vclock component
>> + * to a smaller value, since it would unnecessarily
>> + * require additional WALs, which may have already been
>> + * deleted.
>
> 1. But you just said above, that 0 component is ignored in all
> comparators anyway. So what is a problem of setting 0 component
> to 0?
I didn’t. vclock[0] is ignored in gc_consumer objects now,
but when initializing recovery vclock[0] cannot be ignored.
Otherwise the instance itself would fail to recover local
rows on local recovery.
So relay recovery needs to be initialized with some valid vclock[0]
value.
Valid means «big enough to not require wals that have already been
deleted»
>
>> + * Speaking of gc, remote instances' local vclock
>> + * components are not used by consumers at all.
>> + */
>> + vclock_reset(&replica_clock, 0, vclock_get(&replicaset.vclock, 0));
>> diff --git a/src/box/wal.c b/src/box/wal.c
>> index 3b094b0e8..a74bdecd9 100644
>> --- a/src/box/wal.c
>> +++ b/src/box/wal.c
>> @@ -953,13 +953,19 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>> /** Assign LSN to all local rows. */
>> for ( ; row < end; row++) {
>> if ((*row)->replica_id == 0) {
>> - (*row)->lsn = vclock_inc(vclock_diff, instance_id) +
>> - vclock_get(base, instance_id);
>> /*
>> - * Note, an anonymous replica signs local
>> - * rows whith a zero instance id.
>> + * All rows representing local space data
>> + * manipulations are signed wth a zero
>> + * instance id. This is also true for
>> + * anonymous replicas, since they are
>> + * only capable of writing to local and
>> + * temporary spaces.
>> */
>> - (*row)->replica_id = instance_id;
>> + if ((*row)->group_id != GROUP_LOCAL)
>> + (*row)->replica_id = instance_id;
>> +
>> + (*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) +
>> + vclock_get(base, (*row)->replica_id);
>
> 2. Bad indentation. vlock_get() should be aligned under vclock_inc().
Sorry, my bad. Fixed.
>
>> /* Use lsn of the first local row as transaction id. */
>> tsn = tsn == 0 ? (*row)->lsn : tsn;
>> (*row)->tsn = tsn;
>
> 3. I run the new test without your patch, and it just hangs. No
> xloggaperror like in the ticket. Did it disappear thanks to the
> previous commit, about GC?
Hmm, that’s an interesting bug (or a peculiarity?)
After deleting the old xlog replica is left with only one .snap file,
and doesn’t produce an error regarding «no wal between lsn x and y».
Master is left hanging until replica inserts some new data,
and opens a WAL which cannot be recovered from.
I worked it around.
diff --git a/src/box/wal.c b/src/box/wal.c
index a74bdecd9..1eb20272c 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -965,7 +965,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
(*row)->replica_id = instance_id;
(*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) +
- vclock_get(base, (*row)->replica_id);
+ vclock_get(base, (*row)->replica_id);
/* Use lsn of the first local row as transaction id. */
tsn = tsn == 0 ? (*row)->lsn : tsn;
(*row)->tsn = tsn;
diff --git a/test/replication/gh-4114-local-space-replication.result b/test/replication/gh-4114-local-space-replication.result
index e524c9a1b..4a71bb6dc 100644
--- a/test/replication/gh-4114-local-space-replication.result
+++ b/test/replication/gh-4114-local-space-replication.result
@@ -76,10 +76,7 @@ box.space.test:insert{3}
| ---
| - [3]
| ...
-box.snapshot()
- | ---
- | - ok
- | ...
+
box.info.vclock[0]
| ---
diff --git a/test/replication/gh-4114-local-space-replication.test.lua b/test/replication/gh-4114-local-space-replication.test.lua
index 26dccee68..114c592c5 100644
--- a/test/replication/gh-4114-local-space-replication.test.lua
+++ b/test/replication/gh-4114-local-space-replication.test.lua
@@ -26,7 +26,7 @@ box.snapshot()
box.space.test:insert{2}
box.snapshot()
box.space.test:insert{3}
-box.snapshot()
+
box.info.vclock[0]
--
Serge Petrenko
sergepetrenko at tarantool.org
More information about the Tarantool-patches
mailing list