Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups
@ 2021-04-19  8:37 Cyrill Gorcunov via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode Cyrill Gorcunov via Tarantool-patches
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-19  8:37 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

Hi! Here are a few cleanups for wal and txn_limbo.
The txn_limbo cleanups shrinks code while wal modes
rework allows to save a pointer.

I made the series during work on issue 5447
branch gorcunov/gh-5447-cleanup

Cyrill Gorcunov (3):
  wal: sanitize wal_mode
  txn_limbo: simplify owner migration condition
  txn_limbo: simplify txn_limbo_ack

 src/box/txn_limbo.c |  6 ++----
 src/box/wal.c       |  6 +++++-
 src/box/wal.h       | 27 +++++++++++++++++++++++----
 3 files changed, 30 insertions(+), 9 deletions(-)


base-commit: d2bd451095890c1caaedb2d1c2fc3a49c554915e
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
@ 2021-04-19  8:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-23 20:57   ` Vladislav Shpilevoy via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition Cyrill Gorcunov via Tarantool-patches
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-19  8:37 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

 - there is no need to set WAN_NONE to 0 explicitly
 - add comments about modes
 - there is no need to carry NULL in wal_mode_STRS
 - use designated assignment in wal_mode_STRS

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/wal.c |  6 +++++-
 src/box/wal.h | 27 +++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/src/box/wal.c b/src/box/wal.c
index 34af0bda6..05f30769a 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -55,7 +55,11 @@ enum {
 	WAL_FALLOCATE_LEN = 1024 * 1024,
 };
 
-const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };
+const char *wal_mode_STRS[WAL_MODE_MAX] = {
+	[WAL_NONE]	= "none",
+	[WAL_WRITE]	= "write",
+	[WAL_FSYNC]	= "fsync",
+};
 
 int wal_dir_lock = -1;
 
diff --git a/src/box/wal.h b/src/box/wal.h
index a8a16ba2e..470aa1cc8 100644
--- a/src/box/wal.h
+++ b/src/box/wal.h
@@ -41,7 +41,29 @@ struct fiber;
 struct wal_writer;
 struct tt_uuid;
 
-enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
+enum wal_mode {
+	/**
+	 * Do not write data at all.
+	 */
+	WAL_NONE,
+
+	/**
+	 * Write without waiting the data to be
+	 * flushed to the storage device.
+	 */
+	WAL_WRITE,
+
+	/**
+	 * Write data and wait the record to be
+	 * flushed to the storage device.
+	 */
+	WAL_FSYNC,
+
+	WAL_MODE_MAX
+};
+
+/** String constants for the supported modes. */
+extern const char *wal_mode_STRS[WAL_MODE_MAX];
 
 enum {
 	/**
@@ -52,9 +74,6 @@ enum {
 	WAL_ROWS_PER_YIELD = 1 << 15,
 };
 
-/** String constants for the supported modes. */
-extern const char *wal_mode_STRS[];
-
 extern int wal_dir_lock;
 
 #if defined(__cplusplus)
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode Cyrill Gorcunov via Tarantool-patches
@ 2021-04-19  8:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-25  8:09   ` Serge Petrenko via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack Cyrill Gorcunov via Tarantool-patches
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-19  8:37 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

When limbo owner is about to change the state we should check
if there are some pending transactions which are not yet processed,
iow if queue is empty. No need to test if current limbo owner
is zero. The owner is set to zero once -- when limbo is created
during initialization.

After all I think even if owner would ever zero and we're about
to change it the queue simply must be empty, that is the only
safe state.

Acked-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn_limbo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index addcb0f97..2a10bd0ae 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -94,8 +94,7 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 		id = instance_id;
 	bool make_ro = false;
 	if (limbo->owner_id != id) {
-		if (limbo->owner_id == REPLICA_ID_NIL ||
-		    rlist_empty(&limbo->queue)) {
+		if (rlist_empty(&limbo->queue)) {
 			limbo->owner_id = id;
 			limbo->confirmed_lsn = 0;
 			if (id != instance_id)
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode Cyrill Gorcunov via Tarantool-patches
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition Cyrill Gorcunov via Tarantool-patches
@ 2021-04-19  8:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-25  8:11   ` Serge Petrenko via Tarantool-patches
  2021-04-22 11:34 ` [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change Cyrill Gorcunov via Tarantool-patches
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-19  8:37 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

There is no need to test @confirm_lsn at all because
even with value -1 we're to continue iterating the queue.
Lets drop it and save a branch.

I keep "continue" here to be consistent with other "if"s.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn_limbo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 2a10bd0ae..227b74764 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -505,8 +505,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
 		 */
 		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
 			assert(e->lsn == -1);
-			if (confirm_lsn == -1)
-				continue;
+			continue;
 		} else if (e->lsn <= prev_lsn) {
 			continue;
 		} else if (++e->ack_count < replication_synchro_quorum) {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack Cyrill Gorcunov via Tarantool-patches
@ 2021-04-22 11:34 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-25  8:13   ` Serge Petrenko via Tarantool-patches
  2021-04-25 13:18 ` [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Vladislav Shpilevoy via Tarantool-patches
  2021-04-26 15:40 ` Kirill Yukhin via Tarantool-patches
  5 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-22 11:34 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

There is no need to test @confirm_lsn since we're
continue the list traversing in any case.

I keep "continue" here to be consistent with other "if"s.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn_limbo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 0ac8fff25..c07db3025 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -707,8 +707,7 @@ txn_limbo_on_parameters_change(struct txn_limbo *limbo)
 		assert(e->ack_count <= VCLOCK_MAX);
 		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
 			assert(e->lsn == -1);
-			if (confirm_lsn == -1)
-				continue;
+			continue;
 		} else if (e->ack_count < replication_synchro_quorum) {
 			continue;
 		} else {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode Cyrill Gorcunov via Tarantool-patches
@ 2021-04-23 20:57   ` Vladislav Shpilevoy via Tarantool-patches
  2021-04-23 21:17     ` Cyrill Gorcunov via Tarantool-patches
  0 siblings, 1 reply; 16+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-23 20:57 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Hi! Thanks for the patch!

> diff --git a/src/box/wal.h b/src/box/wal.h
> index a8a16ba2e..470aa1cc8 100644
> --- a/src/box/wal.h
> +++ b/src/box/wal.h
> @@ -41,7 +41,29 @@ struct fiber;
>  struct wal_writer;
>  struct tt_uuid;
>  
> -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
> +enum wal_mode {
> +	/**
> +	 * Do not write data at all.
> +	 */
> +	WAL_NONE,
> +
> +	/**
> +	 * Write without waiting the data to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_WRITE,
> +
> +	/**
> +	 * Write data and wait the record to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_FSYNC,
> +
> +	WAL_MODE_MAX
> +};
> +
> +/** String constants for the supported modes. */
> +extern const char *wal_mode_STRS[WAL_MODE_MAX];

1. Why do you need to declare in the header the array size?
The old version with [] was just fine. And less places to change
if becomes necessary.

2. Please, keep the global variable where it was. We usually group
global variable declarations. It was grouped with wal_dir_lock
below.

>  enum {
>  	/**
> @@ -52,9 +74,6 @@ enum {
>  	WAL_ROWS_PER_YIELD = 1 << 15,
>  };
>  
> -/** String constants for the supported modes. */
> -extern const char *wal_mode_STRS[];
> -
>  extern int wal_dir_lock;
>  
>  #if defined(__cplusplus)
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-23 20:57   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-23 21:17     ` Cyrill Gorcunov via Tarantool-patches
  2021-04-25  8:08       ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-23 21:17 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tml

On Fri, Apr 23, 2021 at 10:57:11PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/wal.h b/src/box/wal.h
> > index a8a16ba2e..470aa1cc8 100644
> > --- a/src/box/wal.h
> > +++ b/src/box/wal.h
> > @@ -41,7 +41,29 @@ struct fiber;
> >  struct wal_writer;
> >  struct tt_uuid;
> >  
> > -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
> > +enum wal_mode {
> > +	/**
> > +	 * Do not write data at all.
> > +	 */
> > +	WAL_NONE,
> > +
> > +	/**
> > +	 * Write without waiting the data to be
> > +	 * flushed to the storage device.
> > +	 */
> > +	WAL_WRITE,
> > +
> > +	/**
> > +	 * Write data and wait the record to be
> > +	 * flushed to the storage device.
> > +	 */
> > +	WAL_FSYNC,
> > +
> > +	WAL_MODE_MAX
> > +};
> > +
> > +/** String constants for the supported modes. */
> > +extern const char *wal_mode_STRS[WAL_MODE_MAX];
> 
> 1. Why do you need to declare in the header the array size?
> The old version with [] was just fine. And less places to change
> if becomes necessary.

If the size is known better to point it explicitly. Thus when you
read this code you will notise the array size (yes, it doesn't
matter from compiler pov if we declare it as a double pointer
but still). But fine I'll leave this declaration untouched.

> 
> 2. Please, keep the global variable where it was. We usually group
> global variable declarations. It was grouped with wal_dir_lock
> below.
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Fri, 16 Apr 2021 15:54:52 +0300
Subject: [PATCH] wal: sanitize wal_mode

 - there is no need to set WAN_NONE to 0 explicitly
 - add comments about modes
 - there is no need to carry NULL in wal_mode_STRS
 - use designated assignment in wal_mode_STRS

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/wal.c |  6 +++++-
 src/box/wal.h | 21 ++++++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/box/wal.c b/src/box/wal.c
index 5b6200b81..6468df884 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -55,7 +55,11 @@ enum {
 	WAL_FALLOCATE_LEN = 1024 * 1024,
 };
 
-const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };
+const char *wal_mode_STRS[WAL_MODE_MAX] = {
+	[WAL_NONE]	= "none",
+	[WAL_WRITE]	= "write",
+	[WAL_FSYNC]	= "fsync",
+};
 
 int wal_dir_lock = -1;
 
diff --git a/src/box/wal.h b/src/box/wal.h
index a8a16ba2e..36fcd39bd 100644
--- a/src/box/wal.h
+++ b/src/box/wal.h
@@ -41,7 +41,26 @@ struct fiber;
 struct wal_writer;
 struct tt_uuid;
 
-enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
+enum wal_mode {
+	/**
+	 * Do not write data at all.
+	 */
+	WAL_NONE,
+
+	/**
+	 * Write without waiting the data to be
+	 * flushed to the storage device.
+	 */
+	WAL_WRITE,
+
+	/**
+	 * Write data and wait the record to be
+	 * flushed to the storage device.
+	 */
+	WAL_FSYNC,
+
+	WAL_MODE_MAX
+};
 
 enum {
 	/**
-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-23 21:17     ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-25  8:08       ` Serge Petrenko via Tarantool-patches
  2021-04-25 13:26         ` Cyrill Gorcunov via Tarantool-patches
  0 siblings, 1 reply; 16+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-25  8:08 UTC (permalink / raw)
  To: Cyrill Gorcunov, Vladislav Shpilevoy; +Cc: tml



24.04.2021 00:17, Cyrill Gorcunov via Tarantool-patches пишет:
> On Fri, Apr 23, 2021 at 10:57:11PM +0200, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>>> diff --git a/src/box/wal.h b/src/box/wal.h
>>> index a8a16ba2e..470aa1cc8 100644
>>> --- a/src/box/wal.h
>>> +++ b/src/box/wal.h
>>> @@ -41,7 +41,29 @@ struct fiber;
>>>   struct wal_writer;
>>>   struct tt_uuid;
>>>   
>>> -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
>>> +enum wal_mode {
>>> +	/**
>>> +	 * Do not write data at all.
>>> +	 */
>>> +	WAL_NONE,
>>> +
>>> +	/**
>>> +	 * Write without waiting the data to be
>>> +	 * flushed to the storage device.
>>> +	 */
>>> +	WAL_WRITE,
>>> +
>>> +	/**
>>> +	 * Write data and wait the record to be
>>> +	 * flushed to the storage device.
>>> +	 */
>>> +	WAL_FSYNC,
>>> +
>>> +	WAL_MODE_MAX
>>> +};
>>> +
>>> +/** String constants for the supported modes. */
>>> +extern const char *wal_mode_STRS[WAL_MODE_MAX];
>> 1. Why do you need to declare in the header the array size?
>> The old version with [] was just fine. And less places to change
>> if becomes necessary.
> If the size is known better to point it explicitly. Thus when you
> read this code you will notise the array size (yes, it doesn't
> matter from compiler pov if we declare it as a double pointer
> but still). But fine I'll leave this declaration untouched.
>
>> 2. Please, keep the global variable where it was. We usually group
>> global variable declarations. It was grouped with wal_dir_lock
>> below.
> ---
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> Date: Fri, 16 Apr 2021 15:54:52 +0300
> Subject: [PATCH] wal: sanitize wal_mode
>
>   - there is no need to set WAN_NONE to 0 explicitly
>   - add comments about modes
>   - there is no need to carry NULL in wal_mode_STRS
>   - use designated assignment in wal_mode_STRS
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/wal.c |  6 +++++-
>   src/box/wal.h | 21 ++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 5b6200b81..6468df884 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -55,7 +55,11 @@ enum {
>   	WAL_FALLOCATE_LEN = 1024 * 1024,
>   };
>   
> -const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };
> +const char *wal_mode_STRS[WAL_MODE_MAX] = {
> +	[WAL_NONE]	= "none",
> +	[WAL_WRITE]	= "write",
> +	[WAL_FSYNC]	= "fsync",
> +};
>   
>   int wal_dir_lock = -1;
>   
> diff --git a/src/box/wal.h b/src/box/wal.h
> index a8a16ba2e..36fcd39bd 100644
> --- a/src/box/wal.h
> +++ b/src/box/wal.h
> @@ -41,7 +41,26 @@ struct fiber;
>   struct wal_writer;
>   struct tt_uuid;
>   
> -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
> +enum wal_mode {
> +	/**
> +	 * Do not write data at all.
> +	 */
> +	WAL_NONE,
> +
> +	/**
> +	 * Write without waiting the data to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_WRITE,
> +
> +	/**
> +	 * Write data and wait the record to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_FSYNC,
> +
> +	WAL_MODE_MAX
> +};
>   

Thanks for the patch!

I'd leave an explicit WAL_NONE = 0.
I didn't know that enums start at zero, TBH.
This would be more readable IMO.

Up to you.

>   enum {
>   	/**

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition Cyrill Gorcunov via Tarantool-patches
@ 2021-04-25  8:09   ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 16+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-25  8:09 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml; +Cc: Vladislav Shpilevoy



19.04.2021 11:37, Cyrill Gorcunov пишет:
> When limbo owner is about to change the state we should check
> if there are some pending transactions which are not yet processed,
> iow if queue is empty. No need to test if current limbo owner
> is zero. The owner is set to zero once -- when limbo is created
> during initialization.
>
> After all I think even if owner would ever zero and we're about
> to change it the queue simply must be empty, that is the only
> safe state.
>
> Acked-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn_limbo.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index addcb0f97..2a10bd0ae 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -94,8 +94,7 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
>   		id = instance_id;
>   	bool make_ro = false;
>   	if (limbo->owner_id != id) {
> -		if (limbo->owner_id == REPLICA_ID_NIL ||
> -		    rlist_empty(&limbo->queue)) {
> +		if (rlist_empty(&limbo->queue)) {
>   			limbo->owner_id = id;
>   			limbo->confirmed_lsn = 0;
>   			if (id != instance_id)
Thanks for the patch! LGTM.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack
  2021-04-19  8:37 ` [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack Cyrill Gorcunov via Tarantool-patches
@ 2021-04-25  8:11   ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 16+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-25  8:11 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml; +Cc: Vladislav Shpilevoy



19.04.2021 11:37, Cyrill Gorcunov пишет:
> There is no need to test @confirm_lsn at all because
> even with value -1 we're to continue iterating the queue.
> Lets drop it and save a branch.
>
> I keep "continue" here to be consistent with other "if"s.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn_limbo.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2a10bd0ae..227b74764 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -505,8 +505,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
>   		 */
>   		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
>   			assert(e->lsn == -1);
> -			if (confirm_lsn == -1)
> -				continue;
> +			continue;
>   		} else if (e->lsn <= prev_lsn) {
>   			continue;
>   		} else if (++e->ack_count < replication_synchro_quorum) {
Thanks! LGTM.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change
  2021-04-22 11:34 ` [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change Cyrill Gorcunov via Tarantool-patches
@ 2021-04-25  8:13   ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 16+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-25  8:13 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml; +Cc: Vladislav Shpilevoy



22.04.2021 14:34, Cyrill Gorcunov пишет:
> There is no need to test @confirm_lsn since we're
> continue the list traversing in any case.
>
> I keep "continue" here to be consistent with other "if"s.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn_limbo.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 0ac8fff25..c07db3025 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -707,8 +707,7 @@ txn_limbo_on_parameters_change(struct txn_limbo *limbo)
>   		assert(e->ack_count <= VCLOCK_MAX);
>   		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
>   			assert(e->lsn == -1);
> -			if (confirm_lsn == -1)
> -				continue;
> +			continue;
>   		} else if (e->ack_count < replication_synchro_quorum) {
>   			continue;
>   		} else {
LGTM.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-04-22 11:34 ` [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change Cyrill Gorcunov via Tarantool-patches
@ 2021-04-25 13:18 ` Vladislav Shpilevoy via Tarantool-patches
  2021-04-26 15:40 ` Kirill Yukhin via Tarantool-patches
  5 siblings, 0 replies; 16+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-25 13:18 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Hi! Thanks for the patchset!

LGTM.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-25  8:08       ` Serge Petrenko via Tarantool-patches
@ 2021-04-25 13:26         ` Cyrill Gorcunov via Tarantool-patches
  2021-04-26  9:40           ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-25 13:26 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: Vladislav Shpilevoy, tml

On Sun, Apr 25, 2021 at 11:08:25AM +0300, Serge Petrenko wrote:
> > +enum wal_mode {
> > +	/**
> > +	 * Do not write data at all.
> > +	 */
> > +	WAL_NONE,
> > +
... 
> Thanks for the patch!
> 
> I'd leave an explicit WAL_NONE = 0.
> I didn't know that enums start at zero, TBH.

Yeah, this is a part of language standart.

> This would be more readable IMO.

I'll make it so and force push. Thanks a huge for review, guys!

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-25 13:26         ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-26  9:40           ` Serge Petrenko via Tarantool-patches
  2021-04-26  9:58             ` Cyrill Gorcunov via Tarantool-patches
  0 siblings, 1 reply; 16+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-04-26  9:40 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: Vladislav Shpilevoy, tml



25.04.2021 16:26, Cyrill Gorcunov пишет:
> On Sun, Apr 25, 2021 at 11:08:25AM +0300, Serge Petrenko wrote:
>>> +enum wal_mode {
>>> +	/**
>>> +	 * Do not write data at all.
>>> +	 */
>>> +	WAL_NONE,
>>> +
> ...
>> Thanks for the patch!
>>
>> I'd leave an explicit WAL_NONE = 0.
>> I didn't know that enums start at zero, TBH.
> Yeah, this is a part of language standart.
>
>> This would be more readable IMO.
> I'll make it so and force push. Thanks a huge for review, guys!

Thanks! Please, update the commit message:
"- there is no need to set WAN_NONE to 0 explicitly" is not true anymore.

LGTM.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode
  2021-04-26  9:40           ` Serge Petrenko via Tarantool-patches
@ 2021-04-26  9:58             ` Cyrill Gorcunov via Tarantool-patches
  0 siblings, 0 replies; 16+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-26  9:58 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: Vladislav Shpilevoy, tml

On Mon, Apr 26, 2021 at 12:40:57PM +0300, Serge Petrenko wrote:
> > I'll make it so and force push. Thanks a huge for review, guys!
> 
> Thanks! Please, update the commit message:
> "- there is no need to set WAN_NONE to 0 explicitly" is not true anymore.

Done! Thanks for noticing.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups
  2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-04-25 13:18 ` [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-26 15:40 ` Kirill Yukhin via Tarantool-patches
  5 siblings, 0 replies; 16+ messages in thread
From: Kirill Yukhin via Tarantool-patches @ 2021-04-26 15:40 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: tml, Vladislav Shpilevoy

Hello,

On 19 апр 11:37, Cyrill Gorcunov wrote:
> Hi! Here are a few cleanups for wal and txn_limbo.
> The txn_limbo cleanups shrinks code while wal modes
> rework allows to save a pointer.
> 
> I made the series during work on issue 5447
> branch gorcunov/gh-5447-cleanup
> 
> Cyrill Gorcunov (3):
>   wal: sanitize wal_mode
>   txn_limbo: simplify owner migration condition
>   txn_limbo: simplify txn_limbo_ack
> 
>  src/box/txn_limbo.c |  6 ++----
>  src/box/wal.c       |  6 +++++-
>  src/box/wal.h       | 27 +++++++++++++++++++++++----
>  3 files changed, 30 insertions(+), 9 deletions(-)

I've checked your patchset into master.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-04-26 15:40 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19  8:37 [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Cyrill Gorcunov via Tarantool-patches
2021-04-19  8:37 ` [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode Cyrill Gorcunov via Tarantool-patches
2021-04-23 20:57   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-23 21:17     ` Cyrill Gorcunov via Tarantool-patches
2021-04-25  8:08       ` Serge Petrenko via Tarantool-patches
2021-04-25 13:26         ` Cyrill Gorcunov via Tarantool-patches
2021-04-26  9:40           ` Serge Petrenko via Tarantool-patches
2021-04-26  9:58             ` Cyrill Gorcunov via Tarantool-patches
2021-04-19  8:37 ` [Tarantool-patches] [PATCH 2/3] txn_limbo: simplify owner migration condition Cyrill Gorcunov via Tarantool-patches
2021-04-25  8:09   ` Serge Petrenko via Tarantool-patches
2021-04-19  8:37 ` [Tarantool-patches] [PATCH 3/3] txn_limbo: simplify txn_limbo_ack Cyrill Gorcunov via Tarantool-patches
2021-04-25  8:11   ` Serge Petrenko via Tarantool-patches
2021-04-22 11:34 ` [Tarantool-patches] [PATCH 4/4] txn_limbo: simplify txn_limbo_on_parameters_change Cyrill Gorcunov via Tarantool-patches
2021-04-25  8:13   ` Serge Petrenko via Tarantool-patches
2021-04-25 13:18 ` [Tarantool-patches] [PATCH 0/3] wal,txn_limbo: a few cleanups Vladislav Shpilevoy via Tarantool-patches
2021-04-26 15:40 ` Kirill Yukhin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox