From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 96BE5469710
 for <tarantool-patches@dev.tarantool.org>;
 Sun,  3 May 2020 21:46:09 +0300 (MSK)
References: <20200428161137.20536-1-gorcunov@gmail.com>
 <20200428161137.20536-9-gorcunov@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <bc344884-0254-d71a-1a6a-dc86b0989c3b@tarantool.org>
Date: Sun, 3 May 2020 20:46:08 +0200
MIME-Version: 1.0
In-Reply-To: <20200428161137.20536-9-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 08/17] recovery:
 recovery_follow_local -- don't throw exception
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index ff095d767..7551ca753 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2292,8 +2292,9 @@ local_recovery(const struct tt_uuid *instance_uuid,
>  	if (wal_dir_lock < 0) {
>  		title("hot_standby");
>  		say_info("Entering hot standby mode");
> -		recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
> -				      cfg_getd("wal_dir_rescan_delay"));
> +		if (recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
> +					  cfg_getd("wal_dir_rescan_delay")) != 0)

1. This becomes out of 80 symbols now. Lets better wrap some arguments.

> +			diag_raise();
>  		while (true) {
>  			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock)) {
>  				recovery_stop_local(recovery);
> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 996966a77..24036b7c1 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -568,9 +568,12 @@ recovery_follow_local(struct recovery *r, struct xstream *stream,
>  	 * xlog.
>  	 */
>  	assert(r->watcher == NULL);
> -	r->watcher = fiber_new_xc(name, hot_standby_f);
> +	r->watcher = fiber_new(name, hot_standby_f);
> +	if (!r->watcher)

2. Lets better follow our code style and use explicit '== NULL'. It looks
more understandable, is consistent (it is compared with NULL literally 2
lines above, and in other places in the other patches of your patchset), and
gives a hint on what type the variable in condition has.

> +		return -1;
>  	fiber_set_joinable(r->watcher, true);
>  	fiber_start(r->watcher, r, stream, wal_dir_rescan_delay);
> +	return 0;
>  }