From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105])
 (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 7FFF5469710
 for <tarantool-patches@dev.tarantool.org>;
 Sun,  3 May 2020 21:44:59 +0300 (MSK)
References: <20200428161137.20536-1-gorcunov@gmail.com>
 <20200428161137.20536-7-gorcunov@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <47c53825-9e6e-607d-9967-8bd95b07d166@tarantool.org>
Date: Sun, 3 May 2020 20:44:51 +0200
MIME-Version: 1.0
In-Reply-To: <20200428161137.20536-7-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 06/17] recovery:
 recover_remaining_wals -- 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/recovery.cc b/src/box/recovery.cc
> index f724600ed..1b78fc915 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -316,14 +316,16 @@ recover_xlog(struct recovery *r, struct xstream *stream,
>   * This function will not close r->current_wal if
>   * recovery was successful.
>   */
> -void
> +int
>  recover_remaining_wals(struct recovery *r, struct xstream *stream,
>  		       const struct vclock *stop_vclock, bool scan_dir)
>  {
>  	struct vclock *clock;
>  
> -	if (scan_dir)
> -		xdir_scan_xc(&r->wal_dir);
> +	if (scan_dir) {
> +		if (xdir_scan(&r->wal_dir) == -1)
> +			return -1;
> +	}

1. Nit: you could make it twice shorter:

	if (scan_dir && xdir_scan(&r->wal_dir) != 0)
		return -1;

> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index 2ad02cb8a..2e17d0476 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -499,8 +501,11 @@ relay_process_wal_event(struct wal_watcher *watcher, unsigned events)
>  		return;
>  	}
>  	try {
> -		recover_remaining_wals(relay->r, &relay->stream, NULL,
> -				       (events & WAL_EVENT_ROTATE) != 0);
> +		bool scan_dir = (events & WAL_EVENT_ROTATE) ? true : false;

2. '!= 0' looked really simpler, and also had boolean type. Besides '!= 0' would
be consistent with other places. For example, when a function result is checked,
we write

	if (func() != 0)
		return -1;

instead of

	if (func() ? true : false)
		return -1;

For flags we also use direct boolean expression assignment. An example in the
same file: recovery.cc, variable scan_dir in hot_standby_f().