[Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Cyrill Gorcunov]

Guys, while I'm working on #5435 and trying to figure out
how to implement it I need to read a lot of code so here
are some patches which I guess make code more readable.

Please take a look and say what you think. We can easily
ignore the whole set.

branch gorcunov/gh-5435-clear_synchro_queue

Cyrill Gorcunov (11):
  build: add more ignore paths for tags target
  vclock: vclock_get - drop misleading masking
  vclock: vclock_inc -- add assert() to catch overflow
  txn: txn_commit_async -- drop redundant variable
  qsync: rename txn_limbo::instance_id to owner_id
  qsync: txn_limbo_append -- use owner_id in argument name
  qsync: move limbo owner transition into separate helper
  qsync: txn_limbo_wait_confirm -- refactor code a bit
  qsync: drop redundant type convention
  relay: use verbose names for fibers
  raft: drop redundant argument

 CMakeLists.txt                 |   6 ++
 src/box/box.cc                 |   2 +-
 src/box/raft.c                 |   2 +-
 src/box/relay.cc               |  17 ++---
 src/box/txn.c                  |  13 ++--
 src/box/txn_limbo.c            | 126 ++++++++++++++++++---------------
 src/box/txn_limbo.h            |   4 +-
 src/lib/vclock/vclock.h        |   7 +-
 test/unit/snap_quorum_delay.cc |   2 +-
 9 files changed, 98 insertions(+), 81 deletions(-)


base-commit: 5d2962244247707c7298a59de0bd4628091dda96
-- 
2.26.2

[Tarantool-patches] [PATCH 01/11] build: add more ignore paths for tags target

[Cyrill Gorcunov]

Our .gitignore has been extended recently lets
add more paths to skip when building the tags
target.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 CMakeLists.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fa6818f8e..84f77dc2b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -150,6 +150,12 @@ check_function_exists(getprogname HAVE_GETPROGNAME)
 list(APPEND tagsExclude "--exclude=.git/*")
 list(APPEND tagsExclude "--exclude=.pc/*")
 list(APPEND tagsExclude "--exclude=patches/*")
+list(APPEND tagsExclude "--exclude=.git-ignore/*")
+list(APPEND tagsExclude "--exclude=coverity/*")
+list(APPEND tagsExclude "--exclude=coverage/*")
+list(APPEND tagsExclude "--exclude=./build/*")
+list(APPEND tagsExclude "--exclude=.idea/*")
+list(APPEND tagsExclude "--exclude=src/module.h")
 add_custom_target(tags COMMAND ${CTAGS} -R ${tagsExclude} -f tags
     WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
 add_custom_target(ctags DEPENDS tags)
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 01/11] build: add more ignore paths for tags target

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 10:51:11PM +0300, Cyrill Gorcunov wrote:
> Our .gitignore has been extended recently lets
> add more paths to skip when building the tags
> target.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Kirill, Vlad or Serge, could you please merge this prticular patch.
I use tags very heavily and since we've updated .gitignore we sould
update exclude paths as well (actually I extended it more to reflect
more paths from current .gitignore).

The rest of series is doable and we might be dropping it.

Re: [Tarantool-patches] [PATCH 01/11] build: add more ignore paths for tags target

[Vladislav Shpilevoy]

Thanks for the patch!

On 12.11.2020 20:51, Cyrill Gorcunov wrote:
> Our .gitignore has been extended recently lets
> add more paths to skip when building the tags
> target.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  CMakeLists.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index fa6818f8e..84f77dc2b 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -150,6 +150,12 @@ check_function_exists(getprogname HAVE_GETPROGNAME)
>  list(APPEND tagsExclude "--exclude=.git/*")
>  list(APPEND tagsExclude "--exclude=.pc/*")
>  list(APPEND tagsExclude "--exclude=patches/*")
> +list(APPEND tagsExclude "--exclude=.git-ignore/*")

What is this? I don't see it locally. So it is not generated, correct?
The same for 'coverity/', 'coverage/', '.idea' - I don't see then appearing
when I compile.

> +list(APPEND tagsExclude "--exclude=coverity/*")
> +list(APPEND tagsExclude "--exclude=coverage/*")
> +list(APPEND tagsExclude "--exclude=./build/*")
> +list(APPEND tagsExclude "--exclude=.idea/*")
> +list(APPEND tagsExclude "--exclude=src/module.h")
>  add_custom_target(tags COMMAND ${CTAGS} -R ${tagsExclude} -f tags
>      WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
>  add_custom_target(ctags DEPENDS tags)

Re: [Tarantool-patches] [PATCH 01/11] build: add more ignore paths for tags target

[Cyrill Gorcunov]

On Sat, Nov 21, 2020 at 12:55:10AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 12.11.2020 20:51, Cyrill Gorcunov wrote:
> > Our .gitignore has been extended recently lets
> > add more paths to skip when building the tags
> > target.
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >  CMakeLists.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index fa6818f8e..84f77dc2b 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -150,6 +150,12 @@ check_function_exists(getprogname HAVE_GETPROGNAME)
> >  list(APPEND tagsExclude "--exclude=.git/*")
> >  list(APPEND tagsExclude "--exclude=.pc/*")
> >  list(APPEND tagsExclude "--exclude=patches/*")
> > +list(APPEND tagsExclude "--exclude=.git-ignore/*")
> 
> What is this? I don't see it locally. So it is not generated, correct?
> The same for 'coverity/', 'coverage/', '.idea' - I don't see then appearing
> when I compile.

These are service directories. Coverity and coverage are for appropriate
tools, .git-ignore for the things you don't want to commit but prefer to
keep in source tree (yes this happens when you need to test something).

[Tarantool-patches] [PATCH 02/11] vclock: vclock_get - drop misleading masking

[Cyrill Gorcunov]

We rely heavily that VCLOCK_MAX is 32 bits wide
and using "VCLOCK_MAX - 1" as a mask for safe
access to the replica id is simply misleading.

Instead use assert here because we might change
the number of supported replicas one day and they
do not have to be pow2 value.

And no need for this completely useless comment.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/vclock/vclock.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/lib/vclock/vclock.h b/src/lib/vclock/vclock.h
index 5865f7443..fd4072c94 100644
--- a/src/lib/vclock/vclock.h
+++ b/src/lib/vclock/vclock.h
@@ -160,11 +160,7 @@ vclock_is_set(const struct vclock *vclock)
 static inline int64_t
 vclock_get(const struct vclock *vclock, uint32_t replica_id)
 {
-	/*
-	 * If replica_id does not fit in VCLOCK_MAX - 1 then
-	 * let result be undefined.
-	 */
-	replica_id &= VCLOCK_MAX - 1;
+	assert(replica_id < VCLOCK_MAX);
 	/* Evaluate a bitmask to avoid branching. */
 	int64_t mask = 0ULL - ((vclock->map >> replica_id) & 0x1);
 	return mask & vclock->lsn[replica_id];
-- 
2.26.2

[Tarantool-patches] [PATCH 03/11] vclock: vclock_inc -- add assert() to catch overflow

[Cyrill Gorcunov]

To make sure we won't access out of bounds in lsn array.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/vclock/vclock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/lib/vclock/vclock.h b/src/lib/vclock/vclock.h
index fd4072c94..09521a393 100644
--- a/src/lib/vclock/vclock.h
+++ b/src/lib/vclock/vclock.h
@@ -169,6 +169,7 @@ vclock_get(const struct vclock *vclock, uint32_t replica_id)
 static inline int64_t
 vclock_inc(struct vclock *vclock, uint32_t replica_id)
 {
+	assert(replica_id < VCLOCK_MAX);
 	/* Easier add each time than check. */
 	if (((vclock->map >> replica_id) & 0x01) == 0) {
 		vclock->lsn[replica_id] = 0;
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 03/11] vclock: vclock_inc -- add assert() to catch overflow

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> To make sure we won't access out of bounds in lsn array.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lib/vclock/vclock.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/lib/vclock/vclock.h b/src/lib/vclock/vclock.h
> index fd4072c94..09521a393 100644
> --- a/src/lib/vclock/vclock.h
> +++ b/src/lib/vclock/vclock.h
> @@ -169,6 +169,7 @@ vclock_get(const struct vclock *vclock, uint32_t replica_id)
>   static inline int64_t
>   vclock_inc(struct vclock *vclock, uint32_t replica_id)
>   {
> +	assert(replica_id < VCLOCK_MAX);
>   	/* Easier add each time than check. */
>   	if (((vclock->map >> replica_id) & 0x01) == 0) {
>   		vclock->lsn[replica_id] = 0;


LGTM

-- 
Serge Petrenko

[Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

We use it once so it is simply redundant.

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

diff --git a/src/box/txn.c b/src/box/txn.c
index a075c9ef0..64f01f4e0 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -815,9 +815,8 @@ txn_commit_async(struct txn *txn)
 	if (req == NULL)
 		goto rollback;
 
-	bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC);
 	struct txn_limbo_entry *limbo_entry;
-	if (is_sync) {
+	if (txn_has_flag(txn, TXN_WAIT_SYNC)) {
 		/*
 		 * We'll need this trigger for sync transactions later,
 		 * but allocation failure is inappropriate after the entry
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> We use it once so it is simply redundant.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/box/txn.c b/src/box/txn.c
> index a075c9ef0..64f01f4e0 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -815,9 +815,8 @@ txn_commit_async(struct txn *txn)
>   	if (req == NULL)
>   		goto rollback;
>   
> -	bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC);
>   	struct txn_limbo_entry *limbo_entry;
> -	if (is_sync) {
> +	if (txn_has_flag(txn, TXN_WAIT_SYNC)) {
>   		/*
>   		 * We'll need this trigger for sync transactions later,
>   		 * but allocation failure is inappropriate after the entry
LGTM

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Vladislav Shpilevoy]

Thanks for the patch!

NACK.

Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

On Sat, Nov 21, 2020 at 12:55:16AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> NACK.

Look, there must be a serious reason why you use temp variable instead
of direct access to the variable itself. They could be

1) The variable might be changed externally after you fetched it
   into another place (which means you might to need a compiler
   barrier as well but it depends, sometime even compiler help
   is not enough and you need hw barrier).

2) The variable is immutable but its name is too long and you use
   temp variable as an alias (which compiler most likely to optimize).

   If code usage in this case is too big then this become unacceptable
   and better redesign the code.

3) It simply happened this way.

The reason 1) and 2) are pretty usable all over the world. In this
particular code we've (3). So I'm asking you "why do you need a temp
variable here?"

When we've been reviewing the whole qsync series I already told
that we better should use direct access to the flags all the
time until there is a reson 1) or 2) to cache the value.

But we've been under the time pressure then and I simply closed
my eyes. Since I'm working on this code right now I hate with
a passion such aggressive caching for NO reason, this even
makes flags grepping more hard - instead of validating flags
access you need to walk two ways: find flag read operation
then find the use of a new variable... and I wonder do you
*really* thing this is normal practice?

Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Vladislav Shpilevoy]

Ok, lets continue arguing.

On 21.11.2020 13:30, Cyrill Gorcunov wrote:
> On Sat, Nov 21, 2020 at 12:55:16AM +0100, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> NACK.
> 
> Look, there must be a serious reason why you use temp variable instead
> of direct access to the variable itself.

Code readability. I consider having it in a variable often more readable.
You don't consider. My word vs yours. Subjective.

But there are real things - by doing such changes you

- turn the git history into garbage by doing non-functional patches
  not changing a single bit of perf or objective readability;

- you waste time on doing this, arguing about this, and time of
  other people on reviewing this. You have real features to implement
  and real bugs to fix. But instead you go around in circles
  changing already working code;

Looks like I told it already. And after I said I won't explain it again,
I did it anyway. I feel stupid about this :) But also I said I won't
commit anything until we all agree on the entire patchset outcome, so
here we go.

> They could be
> 
> 1) The variable might be changed externally after you fetched it
>    into another place (which means you might to need a compiler
>    barrier as well but it depends, sometime even compiler help
>    is not enough and you need hw barrier).

Are you saying this variable existence hurts perf? Then firstly I
ask you to prove that. Show me the bench, on which it matters,
down to 0.0001% of perf improvement. The bench should use Tarantool
and exactly this code. It should not be some kind of example.c you
will create specifically for that with volatiles or stuff. Although
if you can, I could take a look too, not related to Tarantool. Sounds
interesting if it is possible.

Also, since we are here, in the area of crazy blasting real-time
perf, where a single variable introduction matters, lets also talk
about your commit 7/11, where you introduced function txn_limbo_change_owner.
Don't you think we will loose some parts of a nanosecond on doing
a 'call' instruction for this helper? Also the function is a compiler
barrier if the compiler somewhy can't inline it. Don't you think it
may prevent the compiler from doing some reorderings for optimizations?

Also we waste 'huge' time on passing arguments to the function, pushing
them to the stack.

Besides, in case the function is not inlined, it will be stored as a
symbol in the executable file, with its name in it. If we build with
debug info (RelWithDebInfo or Debug). It wastes some bytes to store
the function name.

I propose to inline all functions and remove all variables which are
used one time. To gain the cosmic perf you are talking about. You can
create a ticket, assign it to self, and start immediately.

> 2) The variable is immutable but its name is too long and you use
>    temp variable as an alias (which compiler most likely to optimize).
> 
>    If code usage in this case is too big then this become unacceptable
>    and better redesign the code.
> 
> 3) It simply happened this way.
> 
> The reason 1) and 2) are pretty usable all over the world. In this
> particular code we've (3). So I'm asking you "why do you need a temp
> variable here?"

I am asking you why do you need to change it :)
Because you feel so? It is not a reason. Grepping? The
variable usage is 2 lines below.

Doing such 'optimizations' is fine while a patch is in progress,
if the author does not mind. But changing it in the existing code
I don't consider acceptable.

> When we've been reviewing the whole qsync series I already told
> that we better should use direct access to the flags all the
> time until there is a reson 1) or 2) to cache the value.

And I told you it is subjective. Since there are no real reasons
why I can't do that, it is up to me. As well it is up to you not to
use variables in your patches until it starts hurting readability
or perf.

> But we've been under the time pressure then and I simply closed
> my eyes.

You closed your eyes on me using a variable to save a function
result? Oh, thanks a lot.

> Since I'm working on this code right now I hate with
> a passion such aggressive caching for NO reason, this even
> makes flags grepping more hard - instead of validating flags
> access you need to walk two ways: find flag read operation
> then find the use of a new variable...

It is used literally 2 lines below. If this so hard for you
to locate, I don't know how to help you. But I am not ready to
sacrifice the things I listed above for that - git history and
time. The second one is already lost here, unfortunately, but I
will fight for the first one.

> and I wonder do you
> *really* thing this is normal practice?

It is not a practice. It is not a code style rule. It is just a
fucking variable. The code looks exactly the same before and
after your change.

It can be even that it was used 2 times earlier, but its other
usage was deleted or moved, and the rest was kept intact so as
not to do unnecessary changes.

Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

On Sat, Nov 21, 2020 at 02:29:39PM +0100, Vladislav Shpilevoy wrote:
> Ok, lets continue arguing.
> 
> On 21.11.2020 13:30, Cyrill Gorcunov wrote:
> > On Sat, Nov 21, 2020 at 12:55:16AM +0100, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >> NACK.
> > 
> > Look, there must be a serious reason why you use temp variable instead
> > of direct access to the variable itself.
> 
> Code readability. I consider having it in a variable often more readable.
> You don't consider. My word vs yours. Subjective.

Code readability hits reason (2), ie you make code shorter providing an
alias. Which is prefectly fine. But when someone reads this code first
thing pops up is "why the variable is reassigned?" Maybe we need a local
copy because someone can change it externally while we're operating with
this variable? Looks like nope. Ok, then it must be making code shorter?
No it doesn't either. What is the reason to make an alias, use it once
immediately and that's all :/

> But there are real things - by doing such changes you
> 
> - turn the git history into garbage by doing non-functional patches
>   not changing a single bit of perf or objective readability;

You must be kidding... git history is not a holy cow and never been.
We work with "the code" itself not a git history. This qsync code is
a new one, you don't even have to backport it to 1.x series if there
are some cleanups on top.

> - you waste time on doing this, arguing about this, and time of
>   other people on reviewing this. You have real features to implement
>   and real bugs to fix. But instead you go around in circles
>   changing already working code;

I gave you 2 reasons where vars aliasing is acceptable and needed.
If you really think that "is_sync" is a better name you should consider
just name a bitfield in txn itself instead of declaring new aliases
left and right. This gonna be shorter and won't force a code reader
to scratch the head for the usage context.

> Looks like I told it already. And after I said I won't explain it again,
> I did it anyway. I feel stupid about this :) But also I said I won't
> commit anything until we all agree on the entire patchset outcome, so
> here we go.

You know, we've a different view on the code architecture, this is
understandable. Lets drop this series I can live even with such
code. I continue commenting the message below just in a hope I
could explain you why I did this cleanup but surely won't do
such things in future.

> > They could be
> > 
> > 1) The variable might be changed externally after you fetched it
> >    into another place (which means you might to need a compiler
> >    barrier as well but it depends, sometime even compiler help
> >    is not enough and you need hw barrier).
> 
> Are you saying this variable existence hurts perf? Then firstly I
> ask you to prove that. Show me the bench, on which it matters,

For debug build it makes impossible to track flag read/write
modifications via watching the memory variable on hw level.
And I don't like to disappoint you but _every_ new variable
hurts perf. This is how computers work. Sometime compiler
optimize the usage on release build, sometime not.

The whole patch was about to not procreate new entities where
not needed and keep the code as simple as possible. Even if
it would hurt perf the maintainability reason is a way more
important.

> down to 0.0001% of perf improvement. The bench should use Tarantool
> and exactly this code. It should not be some kind of example.c you
> will create specifically for that with volatiles or stuff. Although
> if you can, I could take a look too, not related to Tarantool. Sounds
> interesting if it is possible.
> 
> Also, since we are here, in the area of crazy blasting real-time
> perf, where a single variable introduction matters, lets also talk
> about your commit 7/11, where you introduced function txn_limbo_change_owner.
> Don't you think we will loose some parts of a nanosecond on doing
> a 'call' instruction for this helper? Also the function is a compiler
> barrier if the compiler somewhy can't inline it. Don't you think it
> may prevent the compiler from doing some reorderings for optimizations?

The txn_limbo_change_owner is special, it moved outside exactly because
changing owner is separate and important state of limbo existance, no?

We need the queue to be empty and I think we will need more tests there
in future because it is very sensitive. Thus even if we have to spend
some time for additional call it should be acceptable in a sake for
further modifications.

If you remember I've been asking you about limbo state in f2f meeting,
and I think it deserves explicit handling even with some additional
cycles.

> Also we waste 'huge' time on passing arguments to the function, pushing
> them to the stack.
>
> Besides, in case the function is not inlined, it will be stored as a
> symbol in the executable file, with its name in it. If we build with
> debug info (RelWithDebInfo or Debug). It wastes some bytes to store
> the function name.
> 
> I propose to inline all functions and remove all variables which are
> used one time. To gain the cosmic perf you are talking about. You can
> create a ticket, assign it to self, and start immediately.

Please don't mixture plain new variables and aliases.

> > 2) The variable is immutable but its name is too long and you use
> >    temp variable as an alias (which compiler most likely to optimize).
> > 
> >    If code usage in this case is too big then this become unacceptable
> >    and better redesign the code.
> > 
> > 3) It simply happened this way.
> > 
> > The reason 1) and 2) are pretty usable all over the world. In this
> > particular code we've (3). So I'm asking you "why do you need a temp
> > variable here?"
> 
> I am asking you why do you need to change it :)

Please read the changelog: we use it once, immediately, no need
for alias. This removes one line of code.

> Because you feel so? It is not a reason. Grepping? The
> variable usage is 2 lines below.
> 
> Doing such 'optimizations' is fine while a patch is in progress,
> if the author does not mind. But changing it in the existing code
> I don't consider acceptable.

I see your position and won't insist.

> > When we've been reviewing the whole qsync series I already told
> > that we better should use direct access to the flags all the
> > time until there is a reson 1) or 2) to cache the value.
> 
> And I told you it is subjective. Since there are no real reasons
> why I can't do that, it is up to me. As well it is up to you not to
> use variables in your patches until it starts hurting readability
> or perf.
> 
> > But we've been under the time pressure then and I simply closed
> > my eyes.
> 
> You closed your eyes on me using a variable to save a function
> result? Oh, thanks a lot.

You're welcome. This was not only one place. The patch I've been
proposing to drop all aliases for flag access. You said somthing
like "it is a cache for flag to not reread it". And I gave up
because we've been in time pressue.

> > Since I'm working on this code right now I hate with
> > a passion such aggressive caching for NO reason, this even
> > makes flags grepping more hard - instead of validating flags
> > access you need to walk two ways: find flag read operation
> > then find the use of a new variable...
> 
> It is used literally 2 lines below. If this so hard for you
> to locate, I don't know how to help you. But I am not ready to

Again, Vlad, it is perfectly fine to have an alias for long
names, here this is made just for nothing. But you already
said "I'm author and I'm to deside", ok, accepted.

> sacrifice the things I listed above for that - git history and
> time. The second one is already lost here, unfortunately, but I
> will fight for the first one.
> 
> > and I wonder do you
> > *really* thing this is normal practice?
> 
> It is not a practice. It is not a code style rule. It is just a
> fucking variable. The code looks exactly the same before and
> after your change.
> 
> It can be even that it was used 2 times earlier, but its other
> usage was deleted or moved, and the rest was kept intact so as
> not to do unnecessary changes.

U+1F926

[Tarantool-patches] [PATCH 05/11] qsync: rename txn_limbo::instance_id to owner_id

[Cyrill Gorcunov]

The instance_id name is too general, we use it
in node's identification while limbo simply "belongs"
to those who tracks current transactions queue.

Lets rename it to owner_id to distinguish from global
instance_id and better grepability.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc                 |  2 +-
 src/box/relay.cc               |  2 +-
 src/box/txn.c                  |  2 +-
 src/box/txn_limbo.c            | 34 +++++++++++++++++-----------------
 src/box/txn_limbo.h            |  2 +-
 test/unit/snap_quorum_delay.cc |  2 +-
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 1f7dec362..9ff4545e1 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1014,7 +1014,7 @@ box_clear_synchro_queue(bool try_wait)
 {
 	if (!is_box_configured || txn_limbo_is_empty(&txn_limbo))
 		return;
-	uint32_t former_leader_id = txn_limbo.instance_id;
+	uint32_t former_leader_id = txn_limbo.owner_id;
 	assert(former_leader_id != REPLICA_ID_NIL);
 	if (former_leader_id == instance_id)
 		return;
diff --git a/src/box/relay.cc b/src/box/relay.cc
index 1e77e0d9b..fa3dc3a75 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -419,7 +419,7 @@ tx_status_update(struct cmsg *msg)
 	 * the single master in 100% so far). Other instances wait
 	 * for master's CONFIRM message instead.
 	 */
-	if (txn_limbo.instance_id == instance_id) {
+	if (txn_limbo.owner_id == instance_id) {
 		txn_limbo_ack(&txn_limbo, status->relay->replica->id,
 			      vclock_get(&status->vclock, instance_id));
 	}
diff --git a/src/box/txn.c b/src/box/txn.c
index 64f01f4e0..f8726026b 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -931,7 +931,7 @@ txn_commit(struct txn *txn)
 			txn_limbo_assign_local_lsn(&txn_limbo, limbo_entry,
 						   lsn);
 			/* Local WAL write is a first 'ACK'. */
-			txn_limbo_ack(&txn_limbo, txn_limbo.instance_id, lsn);
+			txn_limbo_ack(&txn_limbo, txn_limbo.owner_id, lsn);
 		}
 		if (txn_limbo_wait_complete(&txn_limbo, limbo_entry) < 0)
 			goto rollback;
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index c94678885..2c35cd785 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -40,7 +40,7 @@ static inline void
 txn_limbo_create(struct txn_limbo *limbo)
 {
 	rlist_create(&limbo->queue);
-	limbo->instance_id = REPLICA_ID_NIL;
+	limbo->owner_id = REPLICA_ID_NIL;
 	fiber_cond_create(&limbo->wait_cond);
 	vclock_create(&limbo->vclock);
 	limbo->confirmed_lsn = 0;
@@ -72,14 +72,14 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 	}
 	if (id == 0)
 		id = instance_id;
-	if (limbo->instance_id != id) {
-		if (limbo->instance_id == REPLICA_ID_NIL ||
+	if (limbo->owner_id != id) {
+		if (limbo->owner_id == REPLICA_ID_NIL ||
 		    rlist_empty(&limbo->queue)) {
-			limbo->instance_id = id;
+			limbo->owner_id = id;
 			limbo->confirmed_lsn = 0;
 		} else {
 			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
-				 limbo->instance_id);
+				 limbo->owner_id);
 			return NULL;
 		}
 	}
@@ -136,8 +136,8 @@ void
 txn_limbo_assign_remote_lsn(struct txn_limbo *limbo,
 			    struct txn_limbo_entry *entry, int64_t lsn)
 {
-	assert(limbo->instance_id != REPLICA_ID_NIL);
-	assert(limbo->instance_id != instance_id);
+	assert(limbo->owner_id != REPLICA_ID_NIL);
+	assert(limbo->owner_id != instance_id);
 	assert(entry->lsn == -1);
 	assert(lsn > 0);
 	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
@@ -149,8 +149,8 @@ void
 txn_limbo_assign_local_lsn(struct txn_limbo *limbo,
 			   struct txn_limbo_entry *entry, int64_t lsn)
 {
-	assert(limbo->instance_id != REPLICA_ID_NIL);
-	assert(limbo->instance_id == instance_id);
+	assert(limbo->owner_id != REPLICA_ID_NIL);
+	assert(limbo->owner_id == instance_id);
 	assert(entry->lsn == -1);
 	assert(lsn > 0);
 	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
@@ -175,7 +175,7 @@ void
 txn_limbo_assign_lsn(struct txn_limbo *limbo, struct txn_limbo_entry *entry,
 		     int64_t lsn)
 {
-	if (limbo->instance_id == instance_id)
+	if (limbo->owner_id == instance_id)
 		txn_limbo_assign_local_lsn(limbo, entry, lsn);
 	else
 		txn_limbo_assign_remote_lsn(limbo, entry, lsn);
@@ -293,7 +293,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, uint32_t type, int64_t lsn)
 
 	struct synchro_request req = {
 		.type		= type,
-		.replica_id	= limbo->instance_id,
+		.replica_id	= limbo->owner_id,
 		.lsn		= lsn,
 	};
 
@@ -348,7 +348,7 @@ txn_limbo_write_confirm(struct txn_limbo *limbo, int64_t lsn)
 static void
 txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
 {
-	assert(limbo->instance_id != REPLICA_ID_NIL);
+	assert(limbo->owner_id != REPLICA_ID_NIL);
 	struct txn_limbo_entry *e, *tmp;
 	rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
 		/*
@@ -409,7 +409,7 @@ txn_limbo_write_rollback(struct txn_limbo *limbo, int64_t lsn)
 static void
 txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
 {
-	assert(limbo->instance_id != REPLICA_ID_NIL);
+	assert(limbo->owner_id != REPLICA_ID_NIL);
 	struct txn_limbo_entry *e, *tmp;
 	struct txn_limbo_entry *last_rollback = NULL;
 	rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
@@ -471,7 +471,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
 	 */
 	if (limbo->is_in_rollback)
 		return;
-	assert(limbo->instance_id != REPLICA_ID_NIL);
+	assert(limbo->owner_id != REPLICA_ID_NIL);
 	int64_t prev_lsn = vclock_get(&limbo->vclock, replica_id);
 	/*
 	 * One of the reasons why can happen - the remote instance is not
@@ -600,9 +600,9 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
 int
 txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
 {
-	if (req->replica_id != limbo->instance_id) {
-		diag_set(ClientError, ER_SYNC_MASTER_MISMATCH, req->replica_id,
-			 limbo->instance_id);
+	if (req->replica_id != limbo->owner_id) {
+		diag_set(ClientError, ER_SYNC_MASTER_MISMATCH,
+			 req->replica_id, limbo->owner_id);
 		return -1;
 	}
 	switch (req->type) {
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index 3685164a9..c7e70ba64 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -103,7 +103,7 @@ struct txn_limbo {
 	 * won't make sense - different nodes have own independent
 	 * LSNs in their vclock components.
 	 */
-	uint32_t instance_id;
+	uint32_t owner_id;
 	/**
 	 * Condition to wait for completion. It is supposed to be
 	 * signaled when the synchro parameters change. Allowing
diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
index ad0563345..b9d4cc6c4 100644
--- a/test/unit/snap_quorum_delay.cc
+++ b/test/unit/snap_quorum_delay.cc
@@ -131,7 +131,7 @@ txn_process_func(va_list ap)
 	}
 
 	txn_limbo_assign_local_lsn(&txn_limbo, entry, fake_lsn);
-	txn_limbo_ack(&txn_limbo, txn_limbo.instance_id, fake_lsn);
+	txn_limbo_ack(&txn_limbo, txn_limbo.owner_id, fake_lsn);
 	txn_limbo_wait_complete(&txn_limbo, entry);
 
 	switch (process_type) {
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 05/11] qsync: rename txn_limbo::instance_id to owner_id

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> The instance_id name is too general, we use it
> in node's identification while limbo simply "belongs"
> to those who tracks current transactions queue.
>
> Lets rename it to owner_id to distinguish from global
> instance_id and better grepability.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/box.cc                 |  2 +-
>   src/box/relay.cc               |  2 +-
>   src/box/txn.c                  |  2 +-
>   src/box/txn_limbo.c            | 34 +++++++++++++++++-----------------
>   src/box/txn_limbo.h            |  2 +-
>   test/unit/snap_quorum_delay.cc |  2 +-
>   6 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1f7dec362..9ff4545e1 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1014,7 +1014,7 @@ box_clear_synchro_queue(bool try_wait)
>   {
>   	if (!is_box_configured || txn_limbo_is_empty(&txn_limbo))
>   		return;
> -	uint32_t former_leader_id = txn_limbo.instance_id;
> +	uint32_t former_leader_id = txn_limbo.owner_id;
>   	assert(former_leader_id != REPLICA_ID_NIL);
>   	if (former_leader_id == instance_id)
>   		return;
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index 1e77e0d9b..fa3dc3a75 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -419,7 +419,7 @@ tx_status_update(struct cmsg *msg)
>   	 * the single master in 100% so far). Other instances wait
>   	 * for master's CONFIRM message instead.
>   	 */
> -	if (txn_limbo.instance_id == instance_id) {
> +	if (txn_limbo.owner_id == instance_id) {
>   		txn_limbo_ack(&txn_limbo, status->relay->replica->id,
>   			      vclock_get(&status->vclock, instance_id));
>   	}
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 64f01f4e0..f8726026b 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -931,7 +931,7 @@ txn_commit(struct txn *txn)
>   			txn_limbo_assign_local_lsn(&txn_limbo, limbo_entry,
>   						   lsn);
>   			/* Local WAL write is a first 'ACK'. */
> -			txn_limbo_ack(&txn_limbo, txn_limbo.instance_id, lsn);
> +			txn_limbo_ack(&txn_limbo, txn_limbo.owner_id, lsn);
>   		}
>   		if (txn_limbo_wait_complete(&txn_limbo, limbo_entry) < 0)
>   			goto rollback;
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index c94678885..2c35cd785 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -40,7 +40,7 @@ static inline void
>   txn_limbo_create(struct txn_limbo *limbo)
>   {
>   	rlist_create(&limbo->queue);
> -	limbo->instance_id = REPLICA_ID_NIL;
> +	limbo->owner_id = REPLICA_ID_NIL;
>   	fiber_cond_create(&limbo->wait_cond);
>   	vclock_create(&limbo->vclock);
>   	limbo->confirmed_lsn = 0;
> @@ -72,14 +72,14 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
>   	}
>   	if (id == 0)
>   		id = instance_id;
> -	if (limbo->instance_id != id) {
> -		if (limbo->instance_id == REPLICA_ID_NIL ||
> +	if (limbo->owner_id != id) {
> +		if (limbo->owner_id == REPLICA_ID_NIL ||
>   		    rlist_empty(&limbo->queue)) {
> -			limbo->instance_id = id;
> +			limbo->owner_id = id;
>   			limbo->confirmed_lsn = 0;
>   		} else {
>   			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> -				 limbo->instance_id);
> +				 limbo->owner_id);
>   			return NULL;
>   		}
>   	}
> @@ -136,8 +136,8 @@ void
>   txn_limbo_assign_remote_lsn(struct txn_limbo *limbo,
>   			    struct txn_limbo_entry *entry, int64_t lsn)
>   {
> -	assert(limbo->instance_id != REPLICA_ID_NIL);
> -	assert(limbo->instance_id != instance_id);
> +	assert(limbo->owner_id != REPLICA_ID_NIL);
> +	assert(limbo->owner_id != instance_id);
>   	assert(entry->lsn == -1);
>   	assert(lsn > 0);
>   	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
> @@ -149,8 +149,8 @@ void
>   txn_limbo_assign_local_lsn(struct txn_limbo *limbo,
>   			   struct txn_limbo_entry *entry, int64_t lsn)
>   {
> -	assert(limbo->instance_id != REPLICA_ID_NIL);
> -	assert(limbo->instance_id == instance_id);
> +	assert(limbo->owner_id != REPLICA_ID_NIL);
> +	assert(limbo->owner_id == instance_id);
>   	assert(entry->lsn == -1);
>   	assert(lsn > 0);
>   	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
> @@ -175,7 +175,7 @@ void
>   txn_limbo_assign_lsn(struct txn_limbo *limbo, struct txn_limbo_entry *entry,
>   		     int64_t lsn)
>   {
> -	if (limbo->instance_id == instance_id)
> +	if (limbo->owner_id == instance_id)
>   		txn_limbo_assign_local_lsn(limbo, entry, lsn);
>   	else
>   		txn_limbo_assign_remote_lsn(limbo, entry, lsn);
> @@ -293,7 +293,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, uint32_t type, int64_t lsn)
>   
>   	struct synchro_request req = {
>   		.type		= type,
> -		.replica_id	= limbo->instance_id,
> +		.replica_id	= limbo->owner_id,
>   		.lsn		= lsn,
>   	};
>   
> @@ -348,7 +348,7 @@ txn_limbo_write_confirm(struct txn_limbo *limbo, int64_t lsn)
>   static void
>   txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
>   {
> -	assert(limbo->instance_id != REPLICA_ID_NIL);
> +	assert(limbo->owner_id != REPLICA_ID_NIL);
>   	struct txn_limbo_entry *e, *tmp;
>   	rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
>   		/*
> @@ -409,7 +409,7 @@ txn_limbo_write_rollback(struct txn_limbo *limbo, int64_t lsn)
>   static void
>   txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
>   {
> -	assert(limbo->instance_id != REPLICA_ID_NIL);
> +	assert(limbo->owner_id != REPLICA_ID_NIL);
>   	struct txn_limbo_entry *e, *tmp;
>   	struct txn_limbo_entry *last_rollback = NULL;
>   	rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
> @@ -471,7 +471,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
>   	 */
>   	if (limbo->is_in_rollback)
>   		return;
> -	assert(limbo->instance_id != REPLICA_ID_NIL);
> +	assert(limbo->owner_id != REPLICA_ID_NIL);
>   	int64_t prev_lsn = vclock_get(&limbo->vclock, replica_id);
>   	/*
>   	 * One of the reasons why can happen - the remote instance is not
> @@ -600,9 +600,9 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>   int
>   txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>   {
> -	if (req->replica_id != limbo->instance_id) {
> -		diag_set(ClientError, ER_SYNC_MASTER_MISMATCH, req->replica_id,
> -			 limbo->instance_id);
> +	if (req->replica_id != limbo->owner_id) {
> +		diag_set(ClientError, ER_SYNC_MASTER_MISMATCH,
> +			 req->replica_id, limbo->owner_id);
>   		return -1;
>   	}
>   	switch (req->type) {
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 3685164a9..c7e70ba64 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -103,7 +103,7 @@ struct txn_limbo {
>   	 * won't make sense - different nodes have own independent
>   	 * LSNs in their vclock components.
>   	 */
> -	uint32_t instance_id;
> +	uint32_t owner_id;
>   	/**
>   	 * Condition to wait for completion. It is supposed to be
>   	 * signaled when the synchro parameters change. Allowing
> diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
> index ad0563345..b9d4cc6c4 100644
> --- a/test/unit/snap_quorum_delay.cc
> +++ b/test/unit/snap_quorum_delay.cc
> @@ -131,7 +131,7 @@ txn_process_func(va_list ap)
>   	}
>   
>   	txn_limbo_assign_local_lsn(&txn_limbo, entry, fake_lsn);
> -	txn_limbo_ack(&txn_limbo, txn_limbo.instance_id, fake_lsn);
> +	txn_limbo_ack(&txn_limbo, txn_limbo.owner_id, fake_lsn);
>   	txn_limbo_wait_complete(&txn_limbo, entry);
>   
>   	switch (process_type) {
LGTM

-- 
Serge Petrenko

[Tarantool-patches] [PATCH 06/11] qsync: txn_limbo_append -- use owner_id in argument name

[Cyrill Gorcunov]

And change the callers. This id is needed to test for
foreign keys and to set limbo owner on demand.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c       |  8 ++++----
 src/box/txn_limbo.c | 15 ++++++++++-----
 src/box/txn_limbo.h |  2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index f8726026b..d608f6989 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -832,8 +832,8 @@ txn_commit_async(struct txn *txn)
 		}
 
 		/* See txn_commit(). */
-		uint32_t origin_id = req->rows[0]->replica_id;
-		limbo_entry = txn_limbo_append(&txn_limbo, origin_id, txn);
+		uint32_t owner_id = req->rows[0]->replica_id;
+		limbo_entry = txn_limbo_append(&txn_limbo, owner_id, txn);
 		if (limbo_entry == NULL)
 			goto rollback;
 
@@ -900,14 +900,14 @@ txn_commit(struct txn *txn)
 		 * Remote rows, if any, come before local rows, so
 		 * check for originating instance id here.
 		 */
-		uint32_t origin_id = req->rows[0]->replica_id;
+		uint32_t owner_id = req->rows[0]->replica_id;
 
 		/*
 		 * Append now. Before even WAL write is done.
 		 * After WAL write nothing should fail, even OOM
 		 * wouldn't be acceptable.
 		 */
-		limbo_entry = txn_limbo_append(&txn_limbo, origin_id, txn);
+		limbo_entry = txn_limbo_append(&txn_limbo, owner_id, txn);
 		if (limbo_entry == NULL)
 			goto rollback;
 	}
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 2c35cd785..674dcad28 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -49,7 +49,7 @@ txn_limbo_create(struct txn_limbo *limbo)
 }
 
 struct txn_limbo_entry *
-txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
+txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
 {
 	assert(txn_has_flag(txn, TXN_WAIT_SYNC));
 	/*
@@ -70,12 +70,17 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 		diag_set(ClientError, ER_SYNC_ROLLBACK);
 		return NULL;
 	}
-	if (id == 0)
-		id = instance_id;
-	if (limbo->owner_id != id) {
+	if (owner_id == REPLICA_ID_NIL) {
+		/*
+		 * Transactionis initiated on the local node,
+		 * thus we're the owner of the transaction.
+		 */
+		owner_id = instance_id;
+	}
+	if (limbo->owner_id != owner_id) {
 		if (limbo->owner_id == REPLICA_ID_NIL ||
 		    rlist_empty(&limbo->queue)) {
-			limbo->owner_id = id;
+			limbo->owner_id = owner_id;
 			limbo->confirmed_lsn = 0;
 		} else {
 			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index c7e70ba64..1b56903b7 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -191,7 +191,7 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
  * The limbo entry is allocated on the transaction's region.
  */
 struct txn_limbo_entry *
-txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn);
+txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn);
 
 /** Remove the entry from the limbo, mark as rolled back. */
 void
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 06/11] qsync: txn_limbo_append -- use owner_id in argument name

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> And change the callers. This id is needed to test for
> foreign keys and to set limbo owner on demand.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Hi! Thanks for the patch!

Don't do that, please.
Now it looks like you're getting a limbo by its owner id and
insert data there.

This may be the case someday, but now we have a single limbo,
and we may try to insert data with any instance id there (and fail,
if the instance id doesn't match limbo owner id).

>   src/box/txn.c       |  8 ++++----
>   src/box/txn_limbo.c | 15 ++++++++++-----
>   src/box/txn_limbo.h |  2 +-
>   3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/src/box/txn.c b/src/box/txn.c
> index f8726026b..d608f6989 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -832,8 +832,8 @@ txn_commit_async(struct txn *txn)
>   		}
>   
>   		/* See txn_commit(). */
> -		uint32_t origin_id = req->rows[0]->replica_id;
> -		limbo_entry = txn_limbo_append(&txn_limbo, origin_id, txn);
> +		uint32_t owner_id = req->rows[0]->replica_id;
> +		limbo_entry = txn_limbo_append(&txn_limbo, owner_id, txn);
>   		if (limbo_entry == NULL)
>   			goto rollback;
>   
> @@ -900,14 +900,14 @@ txn_commit(struct txn *txn)
>   		 * Remote rows, if any, come before local rows, so
>   		 * check for originating instance id here.
>   		 */
> -		uint32_t origin_id = req->rows[0]->replica_id;
> +		uint32_t owner_id = req->rows[0]->replica_id;
>   
>   		/*
>   		 * Append now. Before even WAL write is done.
>   		 * After WAL write nothing should fail, even OOM
>   		 * wouldn't be acceptable.
>   		 */
> -		limbo_entry = txn_limbo_append(&txn_limbo, origin_id, txn);
> +		limbo_entry = txn_limbo_append(&txn_limbo, owner_id, txn);
>   		if (limbo_entry == NULL)
>   			goto rollback;
>   	}
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2c35cd785..674dcad28 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -49,7 +49,7 @@ txn_limbo_create(struct txn_limbo *limbo)
>   }
>   
>   struct txn_limbo_entry *
> -txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> +txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
>   {


If  you dislike plain "id", call this parameter replica_id instead.


>   	assert(txn_has_flag(txn, TXN_WAIT_SYNC));
>   	/*
> @@ -70,12 +70,17 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
>   		diag_set(ClientError, ER_SYNC_ROLLBACK);
>   		return NULL;
>   	}
> -	if (id == 0)
> -		id = instance_id;
> -	if (limbo->owner_id != id) {
> +	if (owner_id == REPLICA_ID_NIL) {
> +		/*
> +		 * Transactionis initiated on the local node,
> +		 * thus we're the owner of the transaction.
> +		 */
> +		owner_id = instance_id;
> +	}
> +	if (limbo->owner_id != owner_id) {
>   		if (limbo->owner_id == REPLICA_ID_NIL ||
>   		    rlist_empty(&limbo->queue)) {
> -			limbo->owner_id = id;
> +			limbo->owner_id = owner_id;
>   			limbo->confirmed_lsn = 0;
>   		} else {
>   			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index c7e70ba64..1b56903b7 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -191,7 +191,7 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
>    * The limbo entry is allocated on the transaction's region.
>    */
>   struct txn_limbo_entry *
> -txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn);
> +txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn);
>   
>   /** Remove the entry from the limbo, mark as rolled back. */
>   void

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 06/11] qsync: txn_limbo_append -- use owner_id in argument name

[Cyrill Gorcunov]

On Fri, Nov 13, 2020 at 12:43:02PM +0300, Serge Petrenko wrote:
> 
> 12.11.2020 22:51, Cyrill Gorcunov пишет:
> > And change the callers. This id is needed to test for
> > foreign keys and to set limbo owner on demand.
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> 
> Hi! Thanks for the patch!
> 
> Don't do that, please.
> Now it looks like you're getting a limbo by its owner id and
> insert data there.
> 
> This may be the case someday, but now we have a single limbo,
> and we may try to insert data with any instance id there (and fail,
> if the instance id doesn't match limbo owner id).

OK, sure!

Re: [Tarantool-patches] [PATCH 06/11] qsync: txn_limbo_append -- use owner_id in argument name

[Vladislav Shpilevoy]

Thanks for the patch!

After Sergey's comment, NACK.

[Tarantool-patches] [PATCH 07/11] qsync: move limbo owner transition into separate helper

[Cyrill Gorcunov]

This makes transition more explicit since owner_id persistence
while processing limbo queue is a key moment. Same time we
don't need to check for owner_id == REPLICA_ID_NIL since
it can't be nil with nonempty queue.

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

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 674dcad28..b58b9647d 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -48,6 +48,24 @@ txn_limbo_create(struct txn_limbo *limbo)
 	limbo->is_in_rollback = false;
 }
 
+static bool
+txn_limbo_change_owner(struct txn_limbo *limbo, uint32_t owner_id)
+{
+	/*
+	 * Owner transition is allowed for empty txn queue only
+	 * since different nodes have own vclocks and LSNs won't
+	 * be ordered in limbo context.
+	 */
+	if (rlist_empty(&limbo->queue)) {
+		limbo->owner_id = owner_id;
+		limbo->confirmed_lsn = 0;
+		return true;
+	}
+
+	diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS, limbo->owner_id);
+	return false;
+}
+
 struct txn_limbo_entry *
 txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
 {
@@ -77,17 +95,12 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
 		 */
 		owner_id = instance_id;
 	}
+
 	if (limbo->owner_id != owner_id) {
-		if (limbo->owner_id == REPLICA_ID_NIL ||
-		    rlist_empty(&limbo->queue)) {
-			limbo->owner_id = owner_id;
-			limbo->confirmed_lsn = 0;
-		} else {
-			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
-				 limbo->owner_id);
+		if (!txn_limbo_change_owner(limbo, owner_id))
 			return NULL;
-		}
 	}
+
 	size_t size;
 	struct txn_limbo_entry *e = region_alloc_object(&txn->region,
 							typeof(*e), &size);
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 07/11] qsync: move limbo owner transition into separate helper

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> This makes transition more explicit since owner_id persistence
> while processing limbo queue is a key moment. Same time we
> don't need to check for owner_id == REPLICA_ID_NIL since
> it can't be nil with nonempty queue.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn_limbo.c | 29 +++++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 674dcad28..b58b9647d 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -48,6 +48,24 @@ txn_limbo_create(struct txn_limbo *limbo)
>   	limbo->is_in_rollback = false;
>   }
>   
> +static bool
> +txn_limbo_change_owner(struct txn_limbo *limbo, uint32_t owner_id)
> +{
> +	/*
> +	 * Owner transition is allowed for empty txn queue only
> +	 * since different nodes have own vclocks and LSNs won't
> +	 * be ordered in limbo context.
> +	 */
> +	if (rlist_empty(&limbo->queue)) {
> +		limbo->owner_id = owner_id;
> +		limbo->confirmed_lsn = 0;
> +		return true;
> +	}
> +
> +	diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS, limbo->owner_id);
> +	return false;
> +}
> +


AFAIK we use `return 0/-1` for such functions. bool is only used for 
something that
really returns true or false, like `field_mp_type_is_compatible()`.

Other than that the patch looks good.


>   struct txn_limbo_entry *
>   txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
>   {
> @@ -77,17 +95,12 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t owner_id, struct txn *txn)
>   		 */
>   		owner_id = instance_id;
>   	}
> +
>   	if (limbo->owner_id != owner_id) {
> -		if (limbo->owner_id == REPLICA_ID_NIL ||
> -		    rlist_empty(&limbo->queue)) {
> -			limbo->owner_id = owner_id;
> -			limbo->confirmed_lsn = 0;
> -		} else {
> -			diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> -				 limbo->owner_id);
> +		if (!txn_limbo_change_owner(limbo, owner_id))
>   			return NULL;
> -		}
>   	}
> +
>   	size_t size;
>   	struct txn_limbo_entry *e = region_alloc_object(&txn->region,
>   							typeof(*e), &size);

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 07/11] qsync: move limbo owner transition into separate helper

[Cyrill Gorcunov]

On Fri, Nov 13, 2020 at 12:47:59PM +0300, Serge Petrenko wrote:
> 
> AFAIK we use `return 0/-1` for such functions. bool is only used for
> something that
> really returns true or false, like `field_mp_type_is_compatible()`.
> 
> Other than that the patch looks good.

OK, thanks!

Re: [Tarantool-patches] [PATCH 07/11] qsync: move limbo owner transition into separate helper

[Vladislav Shpilevoy]

Thanks for the patch!

NACK.

[Tarantool-patches] [PATCH 08/11] qsync: txn_limbo_wait_confirm -- refactor code a bit

[Cyrill Gorcunov]

 - no need for useless comments which describe what
   we're doing, it is obvious from the code, instead
   put comment explaining "why" we're doing these
   things;
 - use designated assignments for cwp, this is a rule
   of thumb for C where structure need to be initialized
   from the scratch;
 - reorder triggers declaration mess, without empty lines
   which are ordering context they are simply unreadable;
 - try to not use goto/jumps when possible (gotos are
   suitable as a common error entry point but here we
   can handle everything inside cycle itself).

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

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index b58b9647d..cf6122360 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -573,20 +573,25 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
 	if (txn_limbo_is_empty(limbo))
 		return 0;
 
-	/* initialization of a waitpoint. */
-	struct confirm_waitpoint cwp;
-	cwp.caller = fiber();
-	cwp.is_confirm = false;
-	cwp.is_rollback = false;
-
-	/* Set triggers for the last limbo transaction. */
-	struct trigger on_complete;
+	struct confirm_waitpoint cwp = {
+		.caller = fiber(),
+		.is_confirm = false,
+		.is_rollback = false,
+	};
+
+	/*
+	 * Since we're waiting for all sync transactions to complete,
+	 * we need the last entry from the limbo.
+	 */
+	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
+
+	struct trigger on_complete, on_rollback;
 	trigger_create(&on_complete, txn_commit_cb, &cwp, NULL);
-	struct trigger on_rollback;
 	trigger_create(&on_rollback, txn_rollback_cb, &cwp, NULL);
-	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
+
 	txn_on_commit(tle->txn, &on_complete);
 	txn_on_rollback(tle->txn, &on_rollback);
+
 	double start_time = fiber_clock();
 	while (true) {
 		double deadline = start_time + replication_synchro_timeout;
@@ -594,25 +599,18 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
 		double timeout = deadline - fiber_clock();
 		int rc = fiber_cond_wait_timeout(&limbo->wait_cond, timeout);
 		fiber_set_cancellable(cancellable);
-		if (cwp.is_confirm || cwp.is_rollback)
-			goto complete;
-		if (rc != 0)
-			goto timed_out;
-	}
-timed_out:
-	/* Clear the triggers if the timeout has been reached. */
-	trigger_clear(&on_complete);
-	trigger_clear(&on_rollback);
-	diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
-	return -1;
-
-complete:
-	if (!cwp.is_confirm) {
-		/* The transaction has been rolled back. */
-		diag_set(ClientError, ER_SYNC_ROLLBACK);
-		return -1;
+		if (cwp.is_confirm) {
+			return 0;
+		} else if (cwp.is_rollback) {
+			diag_set(ClientError, ER_SYNC_ROLLBACK);
+			return -1;
+		} else if (rc != 0) {
+			trigger_clear(&on_complete);
+			trigger_clear(&on_rollback);
+			diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
+			return -1;
+		}
 	}
-	return 0;
 }
 
 int
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 08/11] qsync: txn_limbo_wait_confirm -- refactor code a bit

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
>   - no need for useless comments which describe what
>     we're doing, it is obvious from the code, instead
>     put comment explaining "why" we're doing these
>     things;
>   - use designated assignments for cwp, this is a rule
>     of thumb for C where structure need to be initialized
>     from the scratch;
>   - reorder triggers declaration mess, without empty lines
>     which are ordering context they are simply unreadable;
>   - try to not use goto/jumps when possible (gotos are
>     suitable as a common error entry point but here we
>     can handle everything inside cycle itself).
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Thanks for the patch!

LGTM

>   src/box/txn_limbo.c | 54 ++++++++++++++++++++++-----------------------
>   1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index b58b9647d..cf6122360 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -573,20 +573,25 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>   	if (txn_limbo_is_empty(limbo))
>   		return 0;
>   
> -	/* initialization of a waitpoint. */
> -	struct confirm_waitpoint cwp;
> -	cwp.caller = fiber();
> -	cwp.is_confirm = false;
> -	cwp.is_rollback = false;
> -
> -	/* Set triggers for the last limbo transaction. */
> -	struct trigger on_complete;
> +	struct confirm_waitpoint cwp = {
> +		.caller = fiber(),
> +		.is_confirm = false,
> +		.is_rollback = false,
> +	};
> +
> +	/*
> +	 * Since we're waiting for all sync transactions to complete,
> +	 * we need the last entry from the limbo.
> +	 */
> +	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
> +
> +	struct trigger on_complete, on_rollback;
>   	trigger_create(&on_complete, txn_commit_cb, &cwp, NULL);
> -	struct trigger on_rollback;
>   	trigger_create(&on_rollback, txn_rollback_cb, &cwp, NULL);
> -	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
> +
>   	txn_on_commit(tle->txn, &on_complete);
>   	txn_on_rollback(tle->txn, &on_rollback);
> +
>   	double start_time = fiber_clock();
>   	while (true) {
>   		double deadline = start_time + replication_synchro_timeout;
> @@ -594,25 +599,18 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>   		double timeout = deadline - fiber_clock();
>   		int rc = fiber_cond_wait_timeout(&limbo->wait_cond, timeout);
>   		fiber_set_cancellable(cancellable);
> -		if (cwp.is_confirm || cwp.is_rollback)
> -			goto complete;
> -		if (rc != 0)
> -			goto timed_out;
> -	}
> -timed_out:
> -	/* Clear the triggers if the timeout has been reached. */
> -	trigger_clear(&on_complete);
> -	trigger_clear(&on_rollback);
> -	diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
> -	return -1;
> -
> -complete:
> -	if (!cwp.is_confirm) {
> -		/* The transaction has been rolled back. */
> -		diag_set(ClientError, ER_SYNC_ROLLBACK);
> -		return -1;
> +		if (cwp.is_confirm) {
> +			return 0;
> +		} else if (cwp.is_rollback) {
> +			diag_set(ClientError, ER_SYNC_ROLLBACK);
> +			return -1;
> +		} else if (rc != 0) {
> +			trigger_clear(&on_complete);
> +			trigger_clear(&on_rollback);
> +			diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
> +			return -1;
> +		}
>   	}
> -	return 0;
>   }
>   
>   int

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 08/11] qsync: txn_limbo_wait_confirm -- refactor code a bit

[Vladislav Shpilevoy]

Thanks for the patch!

NACK.

[Tarantool-patches] [PATCH 09/11] qsync: drop redundant type convention

[Cyrill Gorcunov]

Void pointer can be cast implicitly.

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

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index cf6122360..ffcda389c 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -549,8 +549,7 @@ static int
 txn_commit_cb(struct trigger *trigger, void *event)
 {
 	(void)event;
-	struct confirm_waitpoint *cwp =
-		(struct confirm_waitpoint *)trigger->data;
+	struct confirm_waitpoint *cwp = trigger->data;
 	cwp->is_confirm = true;
 	fiber_wakeup(cwp->caller);
 	return 0;
@@ -560,8 +559,7 @@ static int
 txn_rollback_cb(struct trigger *trigger, void *event)
 {
 	(void)event;
-	struct confirm_waitpoint *cwp =
-		(struct confirm_waitpoint *)trigger->data;
+	struct confirm_waitpoint *cwp = trigger->data;
 	cwp->is_rollback = true;
 	fiber_wakeup(cwp->caller);
 	return 0;
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 09/11] qsync: drop redundant type convention

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> Void pointer can be cast implicitly.

We usually use explicit casts. Check `applier.cc`, `alter.cc` and some 
other places.
This isn't in our style guide though, so I'm fine with the change.

Let's see what Vlad has to say.

>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/txn_limbo.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index cf6122360..ffcda389c 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -549,8 +549,7 @@ static int
>   txn_commit_cb(struct trigger *trigger, void *event)
>   {
>   	(void)event;
> -	struct confirm_waitpoint *cwp =
> -		(struct confirm_waitpoint *)trigger->data;
> +	struct confirm_waitpoint *cwp = trigger->data;
>   	cwp->is_confirm = true;
>   	fiber_wakeup(cwp->caller);
>   	return 0;
> @@ -560,8 +559,7 @@ static int
>   txn_rollback_cb(struct trigger *trigger, void *event)
>   {
>   	(void)event;
> -	struct confirm_waitpoint *cwp =
> -		(struct confirm_waitpoint *)trigger->data;
> +	struct confirm_waitpoint *cwp = trigger->data;
>   	cwp->is_rollback = true;
>   	fiber_wakeup(cwp->caller);
>   	return 0;

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 09/11] qsync: drop redundant type convention

[Cyrill Gorcunov]

On Fri, Nov 13, 2020 at 01:11:05PM +0300, Serge Petrenko wrote:
> 
> 12.11.2020 22:51, Cyrill Gorcunov пишет:
> > Void pointer can be cast implicitly.
> 
> We usually use explicit casts. Check `applier.cc`, `alter.cc` and some other
> places.
> This isn't in our style guide though, so I'm fine with the change.
> 
> Let's see what Vlad has to say.

The casts are required for C++ but for plain C void pointer can be
casted to anything, as it is guaranteed by language standart. So
such casting is simply not needed.

Re: [Tarantool-patches] [PATCH 09/11] qsync: drop redundant type convention

[Serge Petrenko]

13.11.2020 13:13, Cyrill Gorcunov пишет:
> On Fri, Nov 13, 2020 at 01:11:05PM +0300, Serge Petrenko wrote:
>> 12.11.2020 22:51, Cyrill Gorcunov пишет:
>>> Void pointer can be cast implicitly.
>> We usually use explicit casts. Check `applier.cc`, `alter.cc` and some other
>> places.
>> This isn't in our style guide though, so I'm fine with the change.
>>
>> Let's see what Vlad has to say.
> The casts are required for C++ but for plain C void pointer can be
> casted to anything, as it is guaranteed by language standart. So
> such casting is simply not needed.
I see, thanks!
LGTM then.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 09/11] qsync: drop redundant type convention

[Vladislav Shpilevoy]

Thanks for the patch!

NACK.

[Tarantool-patches] [PATCH 10/11] relay: use verbose names for fibers

[Cyrill Gorcunov]

Usually we use _f postfix for fiber's loop functions
and using same postfix for the fiber instance itself
looks inconsistent.

Same time if we grep for "struct fibers" we see a number
of places where fiber instances are postfixed with _fiber.

Thus lets make the same in relay fiber code:

 - use explicit reader_fiber name for a reader
 - use relay_fiber name for the joint relay fiber
   which depends on the reader, moreover this explicit
   name allows to note that the reader cancels the bound
   fiber if error happens.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/relay.cc | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/box/relay.cc b/src/box/relay.cc
index fa3dc3a75..a9522d30c 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -539,7 +539,7 @@ int
 relay_reader_f(va_list ap)
 {
 	struct relay *relay = va_arg(ap, struct relay *);
-	struct fiber *relay_f = va_arg(ap, struct fiber *);
+	struct fiber *relay_fiber = va_arg(ap, struct fiber *);
 
 	struct ibuf ibuf;
 	struct ev_io io;
@@ -557,7 +557,7 @@ relay_reader_f(va_list ap)
 		}
 	} catch (Exception *e) {
 		relay_set_error(relay, e);
-		fiber_cancel(relay_f);
+		fiber_cancel(relay_fiber);
 	}
 	ibuf_destroy(&ibuf);
 	return 0;
@@ -688,9 +688,10 @@ relay_subscribe_f(va_list ap)
 	/* Start fiber for receiving replica acks. */
 	char name[FIBER_NAME_MAX];
 	snprintf(name, sizeof(name), "%s:%s", fiber()->name, "reader");
-	struct fiber *reader = fiber_new_xc(name, relay_reader_f);
-	fiber_set_joinable(reader, true);
-	fiber_start(reader, relay, fiber());
+	struct fiber *reader_fiber = fiber_new_xc(name, relay_reader_f);
+	struct fiber *relay_fiber = fiber();
+	fiber_set_joinable(reader_fiber, true);
+	fiber_start(reader_fiber, relay, relay_fiber);
 
 	/*
 	 * If the replica happens to be up to date on subscribe,
@@ -771,8 +772,8 @@ relay_subscribe_f(va_list ap)
 	wal_clear_watcher(&relay->wal_watcher, cbus_process);
 
 	/* Join ack reader fiber. */
-	fiber_cancel(reader);
-	fiber_join(reader);
+	fiber_cancel(reader_fiber);
+	fiber_join(reader_fiber);
 
 	/* Destroy cpipe to tx. */
 	cbus_unpair(&relay->tx_pipe, &relay->relay_pipe,
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 10/11] relay: use verbose names for fibers

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> Usually we use _f postfix for fiber's loop functions
> and using same postfix for the fiber instance itself
> looks inconsistent.
>
> Same time if we grep for "struct fibers" we see a number
> of places where fiber instances are postfixed with _fiber.
>
> Thus lets make the same in relay fiber code:
>
>   - use explicit reader_fiber name for a reader
>   - use relay_fiber name for the joint relay fiber
>     which depends on the reader, moreover this explicit
>     name allows to note that the reader cancels the bound
>     fiber if error happens.

Applier also has incosistent naming. applier->reader and applier->writer.

Maybe apply the same naming for relay? Make relay_f relay_writer.
And reader_fiber -> relay_reader. It looks better in my opinion.

>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/relay.cc | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index fa3dc3a75..a9522d30c 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -539,7 +539,7 @@ int
>   relay_reader_f(va_list ap)
>   {
>   	struct relay *relay = va_arg(ap, struct relay *);
> -	struct fiber *relay_f = va_arg(ap, struct fiber *);
> +	struct fiber *relay_fiber = va_arg(ap, struct fiber *);
>   
>   	struct ibuf ibuf;
>   	struct ev_io io;
> @@ -557,7 +557,7 @@ relay_reader_f(va_list ap)
>   		}
>   	} catch (Exception *e) {
>   		relay_set_error(relay, e);
> -		fiber_cancel(relay_f);
> +		fiber_cancel(relay_fiber);
>   	}
>   	ibuf_destroy(&ibuf);
>   	return 0;
> @@ -688,9 +688,10 @@ relay_subscribe_f(va_list ap)
>   	/* Start fiber for receiving replica acks. */
>   	char name[FIBER_NAME_MAX];
>   	snprintf(name, sizeof(name), "%s:%s", fiber()->name, "reader");
> -	struct fiber *reader = fiber_new_xc(name, relay_reader_f);
> -	fiber_set_joinable(reader, true);
> -	fiber_start(reader, relay, fiber());
> +	struct fiber *reader_fiber = fiber_new_xc(name, relay_reader_f);
> +	struct fiber *relay_fiber = fiber();
> +	fiber_set_joinable(reader_fiber, true);
> +	fiber_start(reader_fiber, relay, relay_fiber);
>   
>   	/*
>   	 * If the replica happens to be up to date on subscribe,
> @@ -771,8 +772,8 @@ relay_subscribe_f(va_list ap)
>   	wal_clear_watcher(&relay->wal_watcher, cbus_process);
>   
>   	/* Join ack reader fiber. */
> -	fiber_cancel(reader);
> -	fiber_join(reader);
> +	fiber_cancel(reader_fiber);
> +	fiber_join(reader_fiber);
>   
>   	/* Destroy cpipe to tx. */
>   	cbus_unpair(&relay->tx_pipe, &relay->relay_pipe,

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 10/11] relay: use verbose names for fibers

[Cyrill Gorcunov]

On Fri, Nov 13, 2020 at 01:17:29PM +0300, Serge Petrenko wrote:
> 
> 12.11.2020 22:51, Cyrill Gorcunov пишет:
> > Usually we use _f postfix for fiber's loop functions
> > and using same postfix for the fiber instance itself
> > looks inconsistent.
> > 
> > Same time if we grep for "struct fibers" we see a number
> > of places where fiber instances are postfixed with _fiber.
> > 
> > Thus lets make the same in relay fiber code:
> > 
> >   - use explicit reader_fiber name for a reader
> >   - use relay_fiber name for the joint relay fiber
> >     which depends on the reader, moreover this explicit
> >     name allows to note that the reader cancels the bound
> >     fiber if error happens.
> 
> Applier also has incosistent naming. applier->reader and applier->writer.
> 
> Maybe apply the same naming for relay? Make relay_f relay_writer.
> And reader_fiber -> relay_reader. It looks better in my opinion.

Yeah, I though of this as well. Lets drop this patch. Actually I
suspect we gonna withdraw the whole series, except the last patch
where we fix a real bug in say_info for raft. Up to you guys.

Re: [Tarantool-patches] [PATCH 10/11] relay: use verbose names for fibers

[Vladislav Shpilevoy]

Thanks for the patch!

NACK.

[Tarantool-patches] [PATCH 11/11] raft: drop redundant argument

[Cyrill Gorcunov]

It is never used and placed here accidentally.

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

diff --git a/src/box/raft.c b/src/box/raft.c
index ff664a4d1..3f175480c 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -772,7 +772,7 @@ raft_sm_schedule_new_term(struct raft *raft, uint64_t new_term)
 static void
 raft_sm_schedule_new_vote(struct raft *raft, uint32_t new_vote)
 {
-	say_info("RAFT: vote for %u, follow", new_vote, raft->volatile_term);
+	say_info("RAFT: vote for %u, follow", new_vote);
 	assert(raft->volatile_vote == 0);
 	assert(raft->leader == 0);
 	assert(raft->state == RAFT_STATE_FOLLOWER);
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 11/11] raft: drop redundant argument

[Serge Petrenko]

12.11.2020 22:51, Cyrill Gorcunov пишет:
> It is never used and placed here accidentally.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/raft.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/box/raft.c b/src/box/raft.c
> index ff664a4d1..3f175480c 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -772,7 +772,7 @@ raft_sm_schedule_new_term(struct raft *raft, uint64_t new_term)
>   static void
>   raft_sm_schedule_new_vote(struct raft *raft, uint32_t new_vote)
>   {
> -	say_info("RAFT: vote for %u, follow", new_vote, raft->volatile_term);
> +	say_info("RAFT: vote for %u, follow", new_vote);
>   	assert(raft->volatile_vote == 0);
>   	assert(raft->leader == 0);
>   	assert(raft->state == RAFT_STATE_FOLLOWER);
LGTM

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

I won't explain again (third or forth time, I don't remember), why
we don't do refactoring for the sake of refactoring. You can find it
in the mailing list and re-read, if you want. Sorry if sounds rude,
but I give up after explaining it so many times. So I will simply write
NACK for the commits, which literally don't do anything except
subjectively 'more readable' changes.

If you disagree, respond to my emails why concretely. It means you
can prove that the commit fixes a bug, or fixes some serious flaw in
the code style which makes the code look corrupted and totally
unreadable with wrong variable names etc, or it reduces some huge
code duplication. Other things are subjective.

Commits 2, 3, 5, 11 LGTM. I will commit them when the rest of the patches
are reviewed, discussed, and everyone agrees with the decisions.

Re: [Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Vladislav Shpilevoy]

vclock patches pushed to 1.10, 2.5, 2.6, and master.
qsync patch pushed to 2.5, 2.6, and master.
raft patch pushed to 2.6 and master.

Re: [Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Vladislav Shpilevoy]

Hi!

Alexander (Tikh.), please, validate that we can push branch
gorcunov/qsync-refactoring, to which I cherry-picked the
LGTM commits.


Cyrill, for the first commit I don't know what to do.
All the files you listed you use personally in your
workspace, it seems. If we would add everything what we
have locally there, the file would end up as a pile of
garbage. I can't make a decision to push anything new there.
I suggest you to send it to somebody else. For example,
Alexander (Turenko).

Re: [Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Cyrill Gorcunov]

On Tue, Nov 24, 2020 at 12:26:04AM +0100, Vladislav Shpilevoy wrote:
> Hi!
> 
> Alexander (Tikh.), please, validate that we can push branch
> gorcunov/qsync-refactoring, to which I cherry-picked the
> LGTM commits.
> 
> Cyrill, for the first commit I don't know what to do.
> All the files you listed you use personally in your
> workspace, it seems. If we would add everything what we
> have locally there, the file would end up as a pile of
> garbage. I can't make a decision to push anything new there.
> I suggest you to send it to somebody else. For example,
> Alexander (Turenko).

Vlad, I'm pretty fine if we simply drop the series. Sorry
if I was somehow emotional in my replies :)

I think the main reason for series was my attempt to figure
out some limbo internals especially rollback and queue management,
so the series is a side effect. Lets just rip the series off and forget )

Re: [Tarantool-patches] [PATCH 00/11] qsync: code refactoring

[Alexander V. Tikhonov]

Hi Cyrill, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/220256606

On Tue, Nov 24, 2020 at 09:52:47AM +0300, Cyrill Gorcunov wrote:
> On Tue, Nov 24, 2020 at 12:26:04AM +0100, Vladislav Shpilevoy wrote:
> > Hi!
> > 
> > Alexander (Tikh.), please, validate that we can push branch
> > gorcunov/qsync-refactoring, to which I cherry-picked the
> > LGTM commits.
> > 
> > Cyrill, for the first commit I don't know what to do.
> > All the files you listed you use personally in your
> > workspace, it seems. If we would add everything what we
> > have locally there, the file would end up as a pile of
> > garbage. I can't make a decision to push anything new there.
> > I suggest you to send it to somebody else. For example,
> > Alexander (Turenko).
> 
> Vlad, I'm pretty fine if we simply drop the series. Sorry
> if I was somehow emotional in my replies :)
> 
> I think the main reason for series was my attempt to figure
> out some limbo internals especially rollback and queue management,
> so the series is a side effect. Lets just rip the series off and forget )