Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk()
Date: Tue, 15 Jun 2021 23:46:12 +0300
Message-ID: <YMkRlFjtKlqN8skS@grain> (raw)
In-Reply-To: <acd9e680dbe894c570413c395ea49b611821f5d5.1623448465.git.v.shpilevoy@tarantool.org>

On Fri, Jun 11, 2021 at 11:56:12PM +0200, Vladislav Shpilevoy wrote:
> It didn't have a single fail path. That led to some amount of code
> duplication, and it complicated future patches where the journal
> entries are going to get a proper error reason instead of default
> -1 without any details.
> 
> The patch is a preparation for #6027 where it is wanted to have
> more detailed errors on journal entry/transaction fail instead
> of ER_WAL_IO for everything. Sometimes it can override a real
> error like a cascade rollback, or a transaction conflict.
> 
> Part of #6027
> ---
> @@ -1038,7 +1036,10 @@ wal_write_to_disk(struct cmsg *msg)
>  {
>  	struct wal_writer *writer = &wal_writer_singleton;
>  	struct wal_msg *wal_msg = (struct wal_msg *) msg;
>  	struct error *error;
> +	assert(!stailq_empty(&wal_msg->commit));

Hi Vlad, you know I don't understand why we need this assert...

>  	/*
>  	 * Track all vclock changes made by this batch into
> @@ -1058,23 +1059,17 @@ wal_write_to_disk(struct cmsg *msg)
>  
>  	if (writer->is_in_rollback) {
>  		/* We're rolling back a failed write. */
> -		stailq_concat(&wal_msg->rollback, &wal_msg->commit);
> -		vclock_copy(&wal_msg->vclock, &writer->vclock);
> -		return;
> +		goto done;

Jumps to "done" label change the code logic. Before the patch if we
reached the write and say wal_opt_rotate failed we set up is_in_rollback
sign and exit early, after the patch we start notifying watchers that
there "write" happened which means relay code will be woken up while there
no new data on disk level at all, which means watchers wanna be notified
for no reason, no? Or I miss something obvious?

  reply	other threads:[~2021-06-15 20:46 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11 21:56 [Tarantool-patches] [PATCH 00/13] Applier rollback reason Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 01/13] error: introduce ER_CASCADE_ROLLBACK Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 10/13] txn: install proper diag errors on txn fail Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 11/13] wal: introduce JOURNAL_ENTRY_ERR_CASCADE Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 12/13] txn: introduce TXN_SIGNATURE_ABORT Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override Vladislav Shpilevoy via Tarantool-patches
2021-06-15 13:44   ` Serge Petrenko via Tarantool-patches
2021-06-15 19:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 02/13] test: remove replica-applier-rollback.lua Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 03/13] journal: make journal_write() set diag on error Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk() Vladislav Shpilevoy via Tarantool-patches
2021-06-15 20:46   ` Cyrill Gorcunov via Tarantool-patches [this message]
2021-06-16  6:22     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-16  8:02       ` Cyrill Gorcunov via Tarantool-patches
2021-06-16 23:32         ` Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 05/13] diag: introduce diag_set_detailed() Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 06/13] wal: encapsulate ER_WAL_IO Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 07/13] txn: change limbo rollback check in the trigger Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 08/13] journal: introduce proper error codes Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 09/13] txn: assert after WAL write that txn is not done Vladislav Shpilevoy via Tarantool-patches
2021-06-15 13:43 ` [Tarantool-patches] [PATCH 00/13] Applier rollback reason Serge Petrenko via Tarantool-patches
2021-06-16 23:32 ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YMkRlFjtKlqN8skS@grain \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git