Re: [Tarantool-patches] [PATCH 4/5] vclock: ignore 0th component in comparisons.

["Sergey Petrenko" <sergepetrenko@tarantool.org>]

Hi! Thanks for the review.


>Воскресенье, 22 декабря 2019, 20:59 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
>
>Thanks for the patch!
>
>See 2 comments below.
>
>On 15/12/2019 21:58, sergepetrenko wrote:
>> 0th vclock component will be used to count replica-local rows of an
>> anonymous replica. These rows won't be replicated and different
>> instances will have different values in vclock[0]. So ignore 0th
>> component in comparisons.
>
>1. Lets add 'Part of #3186'.
Done.

>
>
>> ---
>>  src/box/vclock.h    | 7 +++++++
>>  test/unit/vclock.cc | 8 ++++----
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/box/vclock.h b/src/box/vclock.h
>> index b5eddcf8b..5dddbd173 100644
>> --- a/src/box/vclock.h
>> +++ b/src/box/vclock.h
>> @@ -281,6 +281,13 @@ vclock_compare(const struct vclock *a, const struct vclock *b)
>> 
>>  for (size_t replica_id = bit_iterator_next(&it); replica_id < VCLOCK_MAX;
>>       replica_id = bit_iterator_next(&it)) {
>> +/*
>> + * Ignore 0-th component in comparisons.
>> + * It is empty for normal replicas and should
>> + * be ignored for anonymous ones.
>> + */
>> +if (replica_id == 0)
>> +continue;
>> 
>>  int64_t lsn_a = vclock_get(a, replica_id);
>>  int64_t lsn_b = vclock_get(b, replica_id);
>
>2. Consider this refactoring in order not to compare replica_id
>with 0 on each iteration:
>
>================================================================================
>
>diff --git a/src/box/vclock.h b/src/box/vclock.h
>index 5dddbd173..fc6aeb724 100644
>--- a/src/box/vclock.h
>+++ b/src/box/vclock.h
>@@ -279,16 +279,15 @@ vclock_compare(const struct vclock *a, const struct vclock *b)
> struct bit_iterator it;
> bit_iterator_init(&it, &map, sizeof(map), true);
>
>-for (size_t replica_id = bit_iterator_next(&it); replica_id < VCLOCK_MAX;
>-     replica_id = bit_iterator_next(&it)) {
>-/*
>- * Ignore 0-th component in comparisons.
>- * It is empty for normal replicas and should
>- * be ignored for anonymous ones.
>- */
>-if (replica_id == 0)
>-continue;
>-
>+size_t replica_id = bit_iterator_next(&it);
>+/*
>+ * Ignore 0-th component in comparisons. It is empty for
>+ * normal replicas and should be ignored for anonymous
>+ * ones.
>+ */
>+if (replica_id == 0)
>+replica_id = bit_iterator_next(&it);
>+for (; replica_id < VCLOCK_MAX; replica_id = bit_iterator_next(&it)) {
> int64_t lsn_a = vclock_get(a, replica_id);
> int64_t lsn_b = vclock_get(b, replica_id);
> le = le && lsn_a <= lsn_b;

Fixed:

diff --git a/src/box/vclock.h b/src/box/vclock.h
index 5dddbd173..8df628c62 100644
--- a/src/box/vclock.h
+++ b/src/box/vclock.h
@@ -279,15 +279,16 @@ vclock_compare(const struct vclock *a, const struct vclock *b)
 struct bit_iterator it;
 bit_iterator_init(&it, &map, sizeof(map), true);
 
+/*
+ * Ignore 0-th component in comparisons.
+ * It is empty for normal replicas and should
+ * be ignored for anonymous ones.
+ */
+if (replica_id == 0)
+replica_id = bit_iterator_next(&it);
+
 for (size_t replica_id = bit_iterator_next(&it); replica_id < VCLOCK_MAX;
      replica_id = bit_iterator_next(&it)) {
-/*
- * Ignore 0-th component in comparisons.
- * It is empty for normal replicas and should
- * be ignored for anonymous ones.
- */
-if (replica_id == 0)
-continue;
 
 int64_t lsn_a = vclock_get(a, replica_id);
 int64_t lsn_b = vclock_get(b, replica_id);

>
>
>================================================================================


-- 
Sergey Petrenko