[Tarantool-patches] [PATCH 00/17] recovery: move from cxx to c code

[Tarantool-patches] [PATCH 00/17] recovery: move from cxx to c code

[Cyrill Gorcunov]

The series is implemented in scope of #3794. The work on
the ticket is still in progress but we definitely have
to move recovery code into C mode one day. So no need
to wait more.

issue https://github.com/tarantool/tarantool/issues/3794
branch gorcunov/gh-3794-recovery-c

Cyrill Gorcunov (16):
  recovery: convert WalSubscription class to structure
  recovery: recovery_close_log -- don't throw exception
  recovery: recovery_open_log -- don't throw exception
  recovery: recover_xlog -- don't throw exception
  recovery: recover_remaining_wals -- don't throw exception
  recovery: hot_standby_f -- don't throw exception
  recovery: recovery_follow_local -- don't throw exception
  recovery: recovery_new -- don't throw exception
  recovery: recovery_scan -- don't throw exception
  recovery: recovery_finalize -- don't throw exception
  recovery: recovery_stop_local -- don't throw exception
  recovery: cxx to c transition
  recovery: drop redundant type_XlogGapError
  recovery: provide throwable wrappers
  box: use _xc helpers of recovery code
  relay: use _xc recovery helpers

Georgy Kirichenko (1):
  recovery: do not call recovery_stop_local inside recovery_delete

 src/box/CMakeLists.txt              |   2 +-
 src/box/box.cc                      |  25 +-
 src/box/{recovery.cc => recovery.c} | 346 ++++++++++++++++------------
 src/box/recovery.h                  |  73 +++++-
 src/box/relay.cc                    |  16 +-
 5 files changed, 287 insertions(+), 175 deletions(-)
 rename src/box/{recovery.cc => recovery.c} (69%)

-- 
2.20.1

[Tarantool-patches] [PATCH 01/17] recovery: do not call recovery_stop_local inside recovery_delete

[Cyrill Gorcunov]

From: Georgy Kirichenko <georgy@tarantool.org>

Recovery stop local raises an exception in case of an recovery error
so it is not safe to stop recovery inside recovery delete and guard
inside local_recovery. So call recovery_stop_local manually.

Part of #980, #3794
---
 src/box/box.cc      | 4 +++-
 src/box/recovery.cc | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index c01b08613..1c6fa582c 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2294,8 +2294,10 @@ local_recovery(const struct tt_uuid *instance_uuid,
 		recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
 				      cfg_getd("wal_dir_rescan_delay"));
 		while (true) {
-			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock))
+			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock)) {
+				recovery_stop_local(recovery);
 				diag_raise();
+			}
 			if (wal_dir_lock >= 0)
 				break;
 			fiber_sleep(0.1);
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index d1a503cfc..1d0e80057 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -216,7 +216,7 @@ recovery_open_log(struct recovery *r, const struct vclock *vclock)
 void
 recovery_delete(struct recovery *r)
 {
-	recovery_stop_local(r);
+	assert(r->watcher == NULL);
 
 	trigger_destroy(&r->on_close_log);
 	xdir_destroy(&r->wal_dir);
-- 
2.20.1

[Tarantool-patches] [PATCH 02/17] recovery: convert WalSubscription class to structure

[Cyrill Gorcunov]

The class is not needed here, we are moving from
cpp to c language. This is a first step to make
recovery code fully C compliant.

Part of #3794

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/recovery.cc | 156 ++++++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 72 deletions(-)

diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 1d0e80057..76b771a91 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -375,83 +375,92 @@ recovery_finalize(struct recovery *r)
  * file triggers a wakeup. The WAL dir path is set in the
  * constructor. XLOG file path is set with set_log_path().
  */
-class WalSubscription {
-public:
-	struct fiber *f;
-	unsigned events;
-	struct ev_stat dir_stat;
-	struct ev_stat file_stat;
-	char dir_path[PATH_MAX];
-	char file_path[PATH_MAX];
-
-	static void dir_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
-	{
-		((WalSubscription *)stat->data)->wakeup(WAL_EVENT_ROTATE);
-	}
-
-	static void file_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
-	{
-		((WalSubscription *)stat->data)->wakeup(WAL_EVENT_WRITE);
-	}
+struct wal_subscr {
+	struct fiber	*f;
+	unsigned int	events;
+	struct ev_stat	dir_stat;
+	struct ev_stat	file_stat;
+	char		dir_path[PATH_MAX];
+	char		file_path[PATH_MAX];
+};
 
-	void wakeup(unsigned events)
-	{
-		this->events |= events;
-		if (f->flags & FIBER_IS_CANCELLABLE)
-			fiber_wakeup(f);
-	}
+static void
+wal_subscr_wakeup(struct wal_subscr *ws, unsigned int events)
+{
+	ws->events |= events;
+	if (ws->f->flags & FIBER_IS_CANCELLABLE)
+		fiber_wakeup(ws->f);
+}
 
-	WalSubscription(const char *wal_dir)
-	{
-		f = fiber();
-		events = 0;
-		if ((size_t)snprintf(dir_path, sizeof(dir_path), "%s", wal_dir) >=
-				sizeof(dir_path)) {
+static void
+wal_subscr_dir_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
+{
+	struct wal_subscr *ws = (struct wal_subscr *)stat->data;
+	wal_subscr_wakeup(ws, WAL_EVENT_ROTATE);
+}
 
-			panic("path too long: %s", wal_dir);
-		}
+static void
+wal_subscr_file_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
+{
+	struct wal_subscr *ws = (struct wal_subscr *)stat->data;
+	wal_subscr_wakeup(ws, WAL_EVENT_WRITE);
+}
 
-		ev_stat_init(&dir_stat, dir_stat_cb, "", 0.0);
-		ev_stat_init(&file_stat, file_stat_cb, "", 0.0);
-		dir_stat.data = this;
-		file_stat.data = this;
+static void
+wal_subscr_set_log_path(struct wal_subscr *ws, const char *path)
+{
+	size_t len;
 
-		ev_stat_set(&dir_stat, dir_path, 0.0);
-		ev_stat_start(loop(), &dir_stat);
+	/*
+	 * Avoid toggling ev_stat if the path didn't change.
+	 * Note: .file_path valid iff file_stat is active.
+	 */
+	if (path && ev_is_active(&ws->file_stat) &&
+	    strcmp(ws->file_path, path) == 0) {
+		return;
 	}
 
-	~WalSubscription()
-	{
-		ev_stat_stop(loop(), &file_stat);
-		ev_stat_stop(loop(), &dir_stat);
-	}
+	ev_stat_stop(loop(), &ws->file_stat);
+	if (path == NULL)
+		return;
 
-	void set_log_path(const char *path)
-	{
-		/*
-		 * Avoid toggling ev_stat if the path didn't change.
-		 * Note: .file_path valid iff file_stat is active.
-		 */
-		if (path && ev_is_active(&file_stat) &&
-				strcmp(file_path, path) == 0) {
+	len = snprintf(ws->file_path, sizeof(ws->file_path), "%s", path);
+	if (len >= sizeof(ws->file_path))
+		panic("wal_subscr: log path is too long: %s", path);
 
-			return;
-		}
+	ev_stat_set(&ws->file_stat, ws->file_path, 0.0);
+	ev_stat_start(loop(), &ws->file_stat);
+}
 
-		ev_stat_stop(loop(), &file_stat);
+static void
+wal_subscr_create(struct wal_subscr *ws, const char *wal_dir)
+{
+	size_t len;
 
-		if (path == NULL)
-			return;
+	memset(ws, 0, sizeof(*ws));
 
-		if ((size_t)snprintf(file_path, sizeof(file_path), "%s", path) >=
-				sizeof(file_path)) {
+	ws->f = fiber();
+	ws->events = 0;
 
-			panic("path too long: %s", path);
-		}
-		ev_stat_set(&file_stat, file_path, 0.0);
-		ev_stat_start(loop(), &file_stat);
-	}
-};
+	len = snprintf(ws->dir_path, sizeof(ws->dir_path), "%s", wal_dir);
+	if (len >= sizeof(ws->dir_path))
+		panic("wal_subscr: wal dir path is too long: %s", wal_dir);
+
+	ev_stat_init(&ws->dir_stat, wal_subscr_dir_stat_cb, "", 0.0);
+	ev_stat_init(&ws->file_stat, wal_subscr_file_stat_cb, "", 0.0);
+	ws->dir_stat.data = ws;
+	ws->file_stat.data = ws;
+
+	ev_stat_set(&ws->dir_stat, ws->dir_path, 0.0);
+	ev_stat_start(loop(), &ws->dir_stat);
+}
+
+static void
+wal_subscr_destroy(struct wal_subscr *ws)
+{
+	ev_stat_stop(loop(), &ws->file_stat);
+	ev_stat_stop(loop(), &ws->dir_stat);
+}
 
 static int
 hot_standby_f(va_list ap)
@@ -463,7 +472,12 @@ hot_standby_f(va_list ap)
 	ev_tstamp wal_dir_rescan_delay = va_arg(ap, ev_tstamp);
 	fiber_set_user(fiber(), &admin_credentials);
 
-	WalSubscription subscription(r->wal_dir.dirname);
+	struct wal_subscr ws;
+	auto guard = make_scoped_guard([&]{
+		wal_subscr_destroy(&ws);
+	});
+
+	wal_subscr_create(&ws, r->wal_dir.dirname);
 
 	while (! fiber_is_cancelled()) {
 
@@ -490,11 +504,11 @@ hot_standby_f(va_list ap)
 			 */
 		} while (end > start && !xlog_cursor_is_open(&r->cursor));
 
-		subscription.set_log_path(xlog_cursor_is_open(&r->cursor) ?
-					  r->cursor.name : NULL);
+		wal_subscr_set_log_path(&ws, xlog_cursor_is_open(&r->cursor) ?
+					r->cursor.name : NULL);
 
 		bool timed_out = false;
-		if (subscription.events == 0) {
+		if (ws.events == 0) {
 			/**
 			 * Allow an immediate wakeup/break loop
 			 * from recovery_stop_local().
@@ -504,10 +518,8 @@ hot_standby_f(va_list ap)
 			fiber_set_cancellable(false);
 		}
 
-		scan_dir = timed_out ||
-			(subscription.events & WAL_EVENT_ROTATE) != 0;
-
-		subscription.events = 0;
+		scan_dir = timed_out || (ws.events & WAL_EVENT_ROTATE) != 0;
+		ws.events = 0;
 	}
 	return 0;
 }
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 02/17] recovery: convert WalSubscription class to structure

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 1d0e80057..76b771a91 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -375,83 +375,92 @@ recovery_finalize(struct recovery *r)
>   * file triggers a wakeup. The WAL dir path is set in the
>   * constructor. XLOG file path is set with set_log_path().

1. Maybe it is worth changing set_log_path() to
wal_subscr_set_log_path().

>   */
> -class WalSubscription {
> -public:
> -	struct fiber *f;
> -	unsigned events;
> -	struct ev_stat dir_stat;
> -	struct ev_stat file_stat;
> -	char dir_path[PATH_MAX];
> -	char file_path[PATH_MAX];
> -
> -	static void dir_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
> -	{
> -		((WalSubscription *)stat->data)->wakeup(WAL_EVENT_ROTATE);
> -	}
> -
> -	static void file_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
> -	{
> -		((WalSubscription *)stat->data)->wakeup(WAL_EVENT_WRITE);
> -	}
> +struct wal_subscr {

2. Lets better not truncate the name. 'subscr' looks ugly.
'wal_subscription' is a fine name, not too long. If you are strictly
against 'subscription', 'wal_sub' would be better too, IMO. At least
it is pronounceable.

> +	struct fiber	*f;
> +	unsigned int	events;
> +	struct ev_stat	dir_stat;
> +	struct ev_stat	file_stat;
> +	char		dir_path[PATH_MAX];
> +	char		file_path[PATH_MAX];

3. Please, don't align struct members. That is not our code style
and creates problems, when a new member is added with a too long
type name.

[Tarantool-patches] [PATCH 03/17] recovery: recovery_close_log -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/recovery.cc | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 76b771a91..16e38cee2 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -144,19 +144,22 @@ recovery_scan(struct recovery *r, struct vclock *end_vclock,
 	xlog_cursor_close(&cursor, false);
 }
 
-static inline void
+static int
 recovery_close_log(struct recovery *r)
 {
-	if (!xlog_cursor_is_open(&r->cursor))
-		return;
-	if (xlog_cursor_is_eof(&r->cursor)) {
-		say_info("done `%s'", r->cursor.name);
-	} else {
-		say_warn("file `%s` wasn't correctly closed",
-			 r->cursor.name);
+	if (xlog_cursor_is_open(&r->cursor)) {
+		if (xlog_cursor_is_eof(&r->cursor)) {
+			say_info("done `%s'", r->cursor.name);
+		} else {
+			say_warn("file `%s` wasn't correctly closed",
+				 r->cursor.name);
+		}
+		xlog_cursor_close(&r->cursor, false);
+
+		if (trigger_run(&r->on_close_log, NULL) != 0)
+			return -1;
 	}
-	xlog_cursor_close(&r->cursor, false);
-	trigger_run_xc(&r->on_close_log, NULL);
+	return 0;
 }
 
 static void
@@ -166,7 +169,8 @@ recovery_open_log(struct recovery *r, const struct vclock *vclock)
 	struct xlog_meta meta = r->cursor.meta;
 	enum xlog_cursor_state state = r->cursor.state;
 
-	recovery_close_log(r);
+	if (recovery_close_log(r) != 0)
+		diag_raise();
 
 	xdir_open_cursor_xc(&r->wal_dir, vclock_sum(vclock), &r->cursor);
 
@@ -349,8 +353,10 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 		recover_xlog(r, stream, stop_vclock);
 	}
 
-	if (xlog_cursor_is_eof(&r->cursor))
-		recovery_close_log(r);
+	if (xlog_cursor_is_eof(&r->cursor)) {
+		if (recovery_close_log(r) != 0)
+			diag_raise();
+	}
 
 	if (stop_vclock != NULL && vclock_compare(&r->vclock, stop_vclock) != 0)
 		tnt_raise(XlogGapError, &r->vclock, stop_vclock);
@@ -361,7 +367,8 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 void
 recovery_finalize(struct recovery *r)
 {
-	recovery_close_log(r);
+	if (recovery_close_log(r) != 0)
+		diag_raise();
 }
 
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 03/17] recovery: recovery_close_log -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 76b771a91..16e38cee2 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -144,19 +144,22 @@ recovery_scan(struct recovery *r, struct vclock *end_vclock,
>  	xlog_cursor_close(&cursor, false);
>  }
>  
> -static inline void
> +static int
>  recovery_close_log(struct recovery *r)
>  {
> -	if (!xlog_cursor_is_open(&r->cursor))
> -		return;
> -	if (xlog_cursor_is_eof(&r->cursor)) {
> -		say_info("done `%s'", r->cursor.name);
> -	} else {
> -		say_warn("file `%s` wasn't correctly closed",
> -			 r->cursor.name);
> +	if (xlog_cursor_is_open(&r->cursor)) {
> +		if (xlog_cursor_is_eof(&r->cursor)) {
> +			say_info("done `%s'", r->cursor.name);
> +		} else {
> +			say_warn("file `%s` wasn't correctly closed",
> +				 r->cursor.name);
> +		}
> +		xlog_cursor_close(&r->cursor, false);
> +
> +		if (trigger_run(&r->on_close_log, NULL) != 0)
> +			return -1;
>  	}
> -	xlog_cursor_close(&r->cursor, false);
> -	trigger_run_xc(&r->on_close_log, NULL);
> +	return 0;
>  }

Seems like you could avoid almost all these changes by simply
this:

====================
-static inline void
+static int
 recovery_close_log(struct recovery *r)
 {
 	if (!xlog_cursor_is_open(&r->cursor))
-		return;
+		return 0;
 	if (xlog_cursor_is_eof(&r->cursor)) {
 		say_info("done `%s'", r->cursor.name);
 	} else {
@@ -156,7 +156,7 @@ recovery_close_log(struct recovery *r)
 			 r->cursor.name);
 	}
 	xlog_cursor_close(&r->cursor, false);
-	trigger_run_xc(&r->on_close_log, NULL);
+	return trigger_run(&r->on_close_log, NULL);
 }

====================

Why so complex?

[Tarantool-patches] [PATCH 04/17] recovery: recovery_open_log -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

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

diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 16e38cee2..55d89903f 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -162,17 +162,18 @@ recovery_close_log(struct recovery *r)
 	return 0;
 }
 
-static void
+static int
 recovery_open_log(struct recovery *r, const struct vclock *vclock)
 {
-	XlogGapError *e;
 	struct xlog_meta meta = r->cursor.meta;
 	enum xlog_cursor_state state = r->cursor.state;
+	int rc = 0;
 
 	if (recovery_close_log(r) != 0)
-		diag_raise();
+		return -1;
 
-	xdir_open_cursor_xc(&r->wal_dir, vclock_sum(vclock), &r->cursor);
+	if (xdir_open_cursor(&r->wal_dir, vclock_sum(vclock), &r->cursor) != 0)
+		return -1;
 
 	if (state == XLOG_CURSOR_NEW &&
 	    vclock_compare(vclock, &r->vclock) > 0) {
@@ -205,15 +206,16 @@ recovery_open_log(struct recovery *r, const struct vclock *vclock)
 	 */
 	if (vclock_compare(&r->vclock, vclock) < 0)
 		vclock_copy(&r->vclock, vclock);
-	return;
+	return rc;
 
 gap_error:
-	e = tnt_error(XlogGapError, &r->vclock, vclock);
-	if (!r->wal_dir.force_recovery)
-		throw e;
-	/* Ignore missing WALs if force_recovery is set. */
-	e->log();
-	say_warn("ignoring a gap in LSN");
+	diag_set(XlogGapError, &r->vclock, vclock);
+	if (r->wal_dir.force_recovery) {
+		diag_log();
+		say_warn("ignoring a gap in LSN");
+	} else {
+		rc = -1;
+	}
 	goto out;
 }
 
@@ -345,7 +347,8 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 			continue;
 		}
 
-		recovery_open_log(r, clock);
+		if (recovery_open_log(r, clock) != 0)
+			diag_raise();
 
 		say_info("recover from `%s'", r->cursor.name);
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 04/17] recovery: recovery_open_log -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 16e38cee2..55d89903f 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -162,17 +162,18 @@ recovery_close_log(struct recovery *r)
>  	return 0;
>  }
>  
> -static void
> +static int
>  recovery_open_log(struct recovery *r, const struct vclock *vclock)
>  {
> -	XlogGapError *e;
>  	struct xlog_meta meta = r->cursor.meta;
>  	enum xlog_cursor_state state = r->cursor.state;
> +	int rc = 0;
>  
>  	if (recovery_close_log(r) != 0)
> -		diag_raise();
> +		return -1;
>  
> -	xdir_open_cursor_xc(&r->wal_dir, vclock_sum(vclock), &r->cursor);

1. The function becomes unused and can be deleted now.

> +	if (xdir_open_cursor(&r->wal_dir, vclock_sum(vclock), &r->cursor) != 0)
> +		return -1;
>  
>  	if (state == XLOG_CURSOR_NEW &&
>  	    vclock_compare(vclock, &r->vclock) > 0) {
> @@ -205,15 +206,16 @@ recovery_open_log(struct recovery *r, const struct vclock *vclock)
>  	 */
>  	if (vclock_compare(&r->vclock, vclock) < 0)
>  		vclock_copy(&r->vclock, vclock);
> -	return;
> +	return rc;
>  
>  gap_error:
> -	e = tnt_error(XlogGapError, &r->vclock, vclock);
> -	if (!r->wal_dir.force_recovery)
> -		throw e;
> -	/* Ignore missing WALs if force_recovery is set. */
> -	e->log();
> -	say_warn("ignoring a gap in LSN");
> +	diag_set(XlogGapError, &r->vclock, vclock);
> +	if (r->wal_dir.force_recovery) {
> +		diag_log();
> +		say_warn("ignoring a gap in LSN");
> +	} else {
> +		rc = -1;

2. By going out here you execute 'vclock_copy(&r->vclock, vclock);'
code line. Which wasn't executed previously, when exception was
thrown above, in 'throw e;' code line.

So I guess you can just do 'return -1;' here, turn 'return rc;' into
'return 0;' above, and remove 'int rc;' at all. If I am not missing
something.

> +	}
>  	goto out;
>  }
>  

[Tarantool-patches] [PATCH 05/17] recovery: recover_xlog -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/recovery.cc | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 55d89903f..f724600ed 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -243,25 +243,28 @@ recovery_delete(struct recovery *r)
  * The reading will be stopped on reaching stop_vclock.
  * Use NULL for boundless recover
  */
-static void
+static int
 recover_xlog(struct recovery *r, struct xstream *stream,
 	     const struct vclock *stop_vclock)
 {
+	bool force_recovery = r->wal_dir.force_recovery;
 	struct xrow_header row;
 	uint64_t row_count = 0;
-	while (xlog_cursor_next_xc(&r->cursor, &row,
-				   r->wal_dir.force_recovery) == 0) {
+	int rc;
+
+	while ((rc = xlog_cursor_next(&r->cursor, &row, force_recovery)) == 0) {
 		/*
 		 * Read the next row from xlog file.
 		 *
-		 * xlog_cursor_next_xc() returns 1 when
+		 * xlog_cursor_next() returns 1 when
 		 * it can not read more rows. This doesn't mean
 		 * the file is fully read: it's fully read only
 		 * when EOF marker has been read, see i.eof_read
 		 */
 		if (stop_vclock != NULL &&
 		    r->vclock.signature >= stop_vclock->signature)
-			return;
+			return 0;
+
 		int64_t current_lsn = vclock_get(&r->vclock, row.replica_id);
 		if (row.lsn <= current_lsn)
 			continue; /* already applied, skip */
@@ -272,6 +275,7 @@ recover_xlog(struct recovery *r, struct xstream *stream,
 		 * are signed with a zero replica id.
 		 */
 		assert(row.replica_id != 0 || row.group_id == GROUP_LOCAL);
+
 		/*
 		 * We can promote the vclock either before or
 		 * after xstream_write(): it only makes any impact
@@ -281,18 +285,24 @@ recover_xlog(struct recovery *r, struct xstream *stream,
 		vclock_follow_xrow(&r->vclock, &row);
 		if (xstream_write(stream, &row) == 0) {
 			++row_count;
-			if (row_count % 100000 == 0)
+			if (row_count % 100000 == 0) {
 				say_info("%.1fM rows processed",
 					 row_count / 1000000.);
-		} else {
-			if (!r->wal_dir.force_recovery)
-				diag_raise();
+			}
+			continue;
+		}
 
-			say_error("skipping row {%u: %lld}",
-				  (unsigned)row.replica_id, (long long)row.lsn);
-			diag_log();
+		if (!force_recovery) {
+			rc = -1;
+			break;
 		}
+
+		say_error("skipping row {%u: %lld}",
+			  (unsigned)row.replica_id, (long long)row.lsn);
+		diag_log();
 	}
+
+	return rc;
 }
 
 /**
@@ -353,7 +363,8 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 		say_info("recover from `%s'", r->cursor.name);
 
 recover_current_wal:
-		recover_xlog(r, stream, stop_vclock);
+		if (recover_xlog(r, stream, stop_vclock) < 0)
+			diag_raise();
 	}
 
 	if (xlog_cursor_is_eof(&r->cursor)) {
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 05/17] recovery: recover_xlog -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 55d89903f..f724600ed 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -243,25 +243,28 @@ recovery_delete(struct recovery *r)
>   * The reading will be stopped on reaching stop_vclock.
>   * Use NULL for boundless recover
>   */
> -static void
> +static int
>  recover_xlog(struct recovery *r, struct xstream *stream,
>  	     const struct vclock *stop_vclock)
>  {
> +	bool force_recovery = r->wal_dir.force_recovery;
>  	struct xrow_header row;
>  	uint64_t row_count = 0;
> -	while (xlog_cursor_next_xc(&r->cursor, &row,
> -				   r->wal_dir.force_recovery) == 0) {

1. This function becomes unused and can be deleted now.

> +	int rc;
> +
> +	while ((rc = xlog_cursor_next(&r->cursor, &row, force_recovery)) == 0) {
> @@ -272,6 +275,7 @@ recover_xlog(struct recovery *r, struct xstream *stream,
>  		 * are signed with a zero replica id.
>  		 */
>  		assert(row.replica_id != 0 || row.group_id == GROUP_LOCAL);
> +

2. Lets better omit unnecessary diff, even if it sometimes looks like
it makes some parts of the code better. At least when it increases
number of diff hunks.

>  		/*
>  		 * We can promote the vclock either before or
>  		 * after xstream_write(): it only makes any impact
> @@ -281,18 +285,24 @@ recover_xlog(struct recovery *r, struct xstream *stream,
>  		vclock_follow_xrow(&r->vclock, &row);
>  		if (xstream_write(stream, &row) == 0) {
>  			++row_count;
> -			if (row_count % 100000 == 0)
> +			if (row_count % 100000 == 0) {
>  				say_info("%.1fM rows processed",
>  					 row_count / 1000000.);
> -		} else {
> -			if (!r->wal_dir.force_recovery)
> -				diag_raise();
> +			}
> +			continue;
> +		}
>  
> -			say_error("skipping row {%u: %lld}",
> -				  (unsigned)row.replica_id, (long long)row.lsn);
> -			diag_log();
> +		if (!force_recovery) {
> +			rc = -1;
> +			break;
>  		}
> +
> +		say_error("skipping row {%u: %lld}",
> +			  (unsigned)row.replica_id, (long long)row.lsn);
> +		diag_log();
>  	}
> +
> +	return rc;

3. Probably better return 0/-1 just like all the other functions, which
can either fail or succeed. Otherwise some new code in future for sure
will check result using '!= 0', and that will be a bug. Because you
can return 1 now.

[Tarantool-patches] [PATCH 06/17] recovery: recover_remaining_wals -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      |  6 ++++--
 src/box/recovery.cc | 24 +++++++++++++++---------
 src/box/recovery.h  |  2 +-
 src/box/relay.cc    | 13 +++++++++----
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 1c6fa582c..ff095d767 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2282,7 +2282,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	memtx_engine_recover_snapshot_xc(memtx, checkpoint_vclock);
 
 	engine_begin_final_recovery_xc();
-	recover_remaining_wals(recovery, &wal_stream.base, NULL, false);
+	if (recover_remaining_wals(recovery, &wal_stream.base, NULL, false) != 0)
+		diag_raise();
 	engine_end_recovery_xc();
 	/*
 	 * Leave hot standby mode, if any, only after
@@ -2303,7 +2304,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
 			fiber_sleep(0.1);
 		}
 		recovery_stop_local(recovery);
-		recover_remaining_wals(recovery, &wal_stream.base, NULL, true);
+		if (recover_remaining_wals(recovery, &wal_stream.base, NULL, true) != 0)
+			diag_raise();
 		/*
 		 * Advance replica set vclock to reflect records
 		 * applied in hot standby mode.
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;
+	}
 
 	if (xlog_cursor_is_open(&r->cursor)) {
 		/* If there's a WAL open, recover from it first. */
@@ -358,24 +360,27 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 		}
 
 		if (recovery_open_log(r, clock) != 0)
-			diag_raise();
+			return -1;
 
 		say_info("recover from `%s'", r->cursor.name);
 
 recover_current_wal:
 		if (recover_xlog(r, stream, stop_vclock) < 0)
-			diag_raise();
+			return -1;
 	}
 
 	if (xlog_cursor_is_eof(&r->cursor)) {
 		if (recovery_close_log(r) != 0)
-			diag_raise();
+			return -1;
 	}
 
-	if (stop_vclock != NULL && vclock_compare(&r->vclock, stop_vclock) != 0)
-		tnt_raise(XlogGapError, &r->vclock, stop_vclock);
+	if (stop_vclock != NULL && vclock_compare(&r->vclock, stop_vclock) != 0) {
+		diag_set(XlogGapError, &r->vclock, stop_vclock);
+		return -1;
+	}
 
 	region_free(&fiber()->gc);
+	return 0;
 }
 
 void
@@ -514,7 +519,8 @@ hot_standby_f(va_list ap)
 		do {
 			start = vclock_sum(&r->vclock);
 
-			recover_remaining_wals(r, stream, NULL, scan_dir);
+			if (recover_remaining_wals(r, stream, NULL, scan_dir) != 0)
+				diag_raise();
 
 			end = vclock_sum(&r->vclock);
 			/*
diff --git a/src/box/recovery.h b/src/box/recovery.h
index 6e68abc0b..a2fb99070 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -102,7 +102,7 @@ recovery_finalize(struct recovery *r);
  * 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);
 
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
@@ -341,8 +341,10 @@ relay_final_join_f(va_list ap)
 
 	/* Send all WALs until stop_vclock */
 	assert(relay->stream.write != NULL);
-	recover_remaining_wals(relay->r, &relay->stream,
-			       &relay->stop_vclock, true);
+	if (recover_remaining_wals(relay->r, &relay->stream,
+				   &relay->stop_vclock, true) != 0) {
+		diag_raise();
+	}
 	assert(vclock_compare(&relay->r->vclock, &relay->stop_vclock) == 0);
 	return 0;
 }
@@ -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;
+		if (recover_remaining_wals(relay->r, &relay->stream, NULL,
+					   scan_dir) != 0) {
+			diag_raise();
+		}
 	} catch (Exception *e) {
 		relay_set_error(relay, e);
 		fiber_cancel(fiber());
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 06/17] recovery: recover_remaining_wals -- don't throw exception

[Vladislav Shpilevoy]

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().

[Tarantool-patches] [PATCH 07/17] recovery: hot_standby_f -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

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

diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 1b78fc915..996966a77 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -494,18 +494,15 @@ hot_standby_f(va_list ap)
 	struct recovery *r = va_arg(ap, struct recovery *);
 	struct xstream *stream = va_arg(ap, struct xstream *);
 	bool scan_dir = true;
+	int rc = 0;
 
 	ev_tstamp wal_dir_rescan_delay = va_arg(ap, ev_tstamp);
 	fiber_set_user(fiber(), &admin_credentials);
 
 	struct wal_subscr ws;
-	auto guard = make_scoped_guard([&]{
-		wal_subscr_destroy(&ws);
-	});
-
 	wal_subscr_create(&ws, r->wal_dir.dirname);
 
-	while (! fiber_is_cancelled()) {
+	while (!fiber_is_cancelled()) {
 
 		/*
 		 * Recover until there is no new stuff which appeared in
@@ -519,8 +516,16 @@ hot_standby_f(va_list ap)
 		do {
 			start = vclock_sum(&r->vclock);
 
-			if (recover_remaining_wals(r, stream, NULL, scan_dir) != 0)
-				diag_raise();
+			if (recover_remaining_wals(r, stream, NULL, scan_dir) != 0) {
+				/*
+				 * Since we're the fiber function the wrapper
+				 * fiber_cxx_invoke doesn't log the real reson
+				 * of the failure. Thus make it so explicitly.
+				 */
+				diag_log();
+				rc = -1;
+				goto out;
+			}
 
 			end = vclock_sum(&r->vclock);
 			/*
@@ -548,7 +553,9 @@ hot_standby_f(va_list ap)
 		scan_dir = timed_out || (ws.events & WAL_EVENT_ROTATE) != 0;
 		ws.events = 0;
 	}
-	return 0;
+out:
+	wal_subscr_destroy(&ws);
+	return rc;
 }
 
 void
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 07/17] recovery: hot_standby_f -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

On 28/04/2020 18:11, Cyrill Gorcunov wrote:
> Prepare for transition to plain C.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/recovery.cc | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 1b78fc915..996966a77 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -494,18 +494,15 @@ hot_standby_f(va_list ap)
>  	struct recovery *r = va_arg(ap, struct recovery *);
>  	struct xstream *stream = va_arg(ap, struct xstream *);
>  	bool scan_dir = true;
> +	int rc = 0;
>  
>  	ev_tstamp wal_dir_rescan_delay = va_arg(ap, ev_tstamp);
>  	fiber_set_user(fiber(), &admin_credentials);
>  
>  	struct wal_subscr ws;
> -	auto guard = make_scoped_guard([&]{
> -		wal_subscr_destroy(&ws);
> -	});
> -
>  	wal_subscr_create(&ws, r->wal_dir.dirname);
>  
> -	while (! fiber_is_cancelled()) {
> +	while (!fiber_is_cancelled()) {

I know that sometimes you may really want to change something,
that looks 'wrong' even when it is not related to the patch, but
please, lets better avoid such changes. They not only make the
patch bigger complicating the review, but also make it harder to
navigate the code in 'git blame', when you want to discover why
something was done and by whom.

[Tarantool-patches] [PATCH 08/17] recovery: recovery_follow_local -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      | 5 +++--
 src/box/recovery.cc | 7 +++++--
 src/box/recovery.h  | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

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)
+			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
@@ -558,7 +558,7 @@ hot_standby_f(va_list ap)
 	return rc;
 }
 
-void
+int
 recovery_follow_local(struct recovery *r, struct xstream *stream,
 		      const char *name, ev_tstamp wal_dir_rescan_delay)
 {
@@ -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)
+		return -1;
 	fiber_set_joinable(r->watcher, true);
 	fiber_start(r->watcher, r, stream, wal_dir_rescan_delay);
+	return 0;
 }
 
 void
diff --git a/src/box/recovery.h b/src/box/recovery.h
index a2fb99070..7aa496b14 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -78,7 +78,7 @@ void
 recovery_scan(struct recovery *r,  struct vclock *end_vclock,
 	      struct vclock *gc_vclock);
 
-void
+int
 recovery_follow_local(struct recovery *r, struct xstream *stream,
 		      const char *name, ev_tstamp wal_dir_rescan_delay);
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 08/17] recovery: recovery_follow_local -- don't throw exception

[Vladislav Shpilevoy]

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;
>  }

[Tarantool-patches] [PATCH 09/17] recovery: recovery_new -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      |  2 ++
 src/box/recovery.cc | 22 ++++++++++------------
 src/box/relay.cc    | 12 ++++++++----
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 7551ca753..4371c1986 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2224,6 +2224,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	recovery = recovery_new(cfg_gets("wal_dir"),
 				cfg_geti("force_recovery"),
 				checkpoint_vclock);
+	if (recovery == NULL)
+		diag_raise();
 
 	/*
 	 * Make sure we report the actual recovery position
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 24036b7c1..1c7665f87 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -77,24 +77,21 @@
 /* {{{ Initial recovery */
 
 /**
- * Throws an exception in  case of error.
+ * Returns NULL in case of error.
  */
 struct recovery *
 recovery_new(const char *wal_dirname, bool force_recovery,
 	     const struct vclock *vclock)
 {
-	struct recovery *r = (struct recovery *)
-			calloc(1, sizeof(*r));
+	struct recovery *r;
 
+	r = (struct recovery *)calloc(1, sizeof(*r));
 	if (r == NULL) {
-		tnt_raise(OutOfMemory, sizeof(*r), "malloc",
-			  "struct recovery");
+		diag_set(OutOfMemory, sizeof(*r), "calloc",
+			 "struct recovery");
+		return NULL;
 	}
 
-	auto guard = make_scoped_guard([=]{
-		free(r);
-	});
-
 	xdir_create(&r->wal_dir, wal_dirname, XLOG, &INSTANCE_UUID,
 		    &xlog_opts_default);
 	r->wal_dir.force_recovery = force_recovery;
@@ -108,12 +105,13 @@ recovery_new(const char *wal_dirname, bool force_recovery,
 	 * UUID, see replication/cluster.test for
 	 * details.
 	 */
-	xdir_check_xc(&r->wal_dir);
+	if (xdir_check(&r->wal_dir) != 0) {
+		free(r);
+		return NULL;
+	}
 
 	r->watcher = NULL;
 	rlist_create(&r->on_close_log);
-
-	guard.is_active = false;
 	return r;
 }
 
diff --git a/src/box/relay.cc b/src/box/relay.cc
index 2e17d0476..a5bcd9580 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -363,8 +363,10 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
 		relay_delete(relay);
 	});
 
-	relay->r = recovery_new(cfg_gets("wal_dir"), false,
-			       start_vclock);
+	relay->r = recovery_new(cfg_gets("wal_dir"), false, start_vclock);
+	if (relay->r == NULL)
+		diag_raise();
+
 	vclock_copy(&relay->stop_vclock, stop_vclock);
 
 	int rc = cord_costart(&relay->cord, "final_join",
@@ -717,8 +719,10 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
 	});
 
 	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
-	relay->r = recovery_new(cfg_gets("wal_dir"), false,
-			        replica_clock);
+	relay->r = recovery_new(cfg_gets("wal_dir"), false, replica_clock);
+	if (relay->r == NULL)
+		diag_raise();
+
 	vclock_copy(&relay->tx.vclock, replica_clock);
 	relay->version_id = replica_version_id;
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 09/17] recovery: recovery_new -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 24036b7c1..1c7665f87 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -77,24 +77,21 @@
>  /* {{{ Initial recovery */
>  
>  /**
> - * Throws an exception in  case of error.
> + * Returns NULL in case of error.
>   */
>  struct recovery *
>  recovery_new(const char *wal_dirname, bool force_recovery,
>  	     const struct vclock *vclock)
>  {
> -	struct recovery *r = (struct recovery *)
> -			calloc(1, sizeof(*r));
> +	struct recovery *r;
>  
> +	r = (struct recovery *)calloc(1, sizeof(*r));

1. Would look better, if you assign 'r' on the same line
where you declare it. It is not really necessary to declare
all variables in the beginning and assign them separately.
Looks ugly, IMO. At least this particular place.

> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index 2e17d0476..a5bcd9580 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -363,8 +363,10 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
>  		relay_delete(relay);
>  	});
>  
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			       start_vclock);
> +	relay->r = recovery_new(cfg_gets("wal_dir"), false, start_vclock);
> +	if (relay->r == NULL)
> +		diag_raise();
> +
>  	vclock_copy(&relay->stop_vclock, stop_vclock);
>  
>  	int rc = cord_costart(&relay->cord, "final_join",
> @@ -717,8 +719,10 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
>  	});
>  
>  	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			        replica_clock);

2. This may look like a nit, but you could keep these 2 lines unchanged.
The same about the hunk above. My arguments are the same - patch size and
git blame purity.

> +	relay->r = recovery_new(cfg_gets("wal_dir"), false, replica_clock);
> +	if (relay->r == NULL)
> +		diag_raise();
> +
>  	vclock_copy(&relay->tx.vclock, replica_clock);
>  	relay->version_id = replica_version_id;
>  
> 

[Tarantool-patches] [PATCH 10/17] recovery: recovery_scan -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      |  3 ++-
 src/box/recovery.cc | 18 +++++++++++++-----
 src/box/recovery.h  |  2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 4371c1986..c269a5da8 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2243,7 +2243,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	 * so we must reflect this in replicaset vclock to
 	 * not attempt to apply these rows twice.
 	 */
-	recovery_scan(recovery, &replicaset.vclock, &gc.vclock);
+	if (recovery_scan(recovery, &replicaset.vclock, &gc.vclock) != 0)
+		diag_raise();
 	say_info("instance vclock %s", vclock_to_string(&replicaset.vclock));
 
 	if (wal_dir_lock >= 0) {
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 1c7665f87..af7910bdf 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -115,18 +115,19 @@ recovery_new(const char *wal_dirname, bool force_recovery,
 	return r;
 }
 
-void
+int
 recovery_scan(struct recovery *r, struct vclock *end_vclock,
 	      struct vclock *gc_vclock)
 {
-	xdir_scan_xc(&r->wal_dir);
+	if (xdir_scan(&r->wal_dir) == -1)
+		return -1;
 
 	if (xdir_last_vclock(&r->wal_dir, end_vclock) < 0 ||
 	    vclock_compare(end_vclock, &r->vclock) < 0) {
 		/* No xlogs after last checkpoint. */
 		vclock_copy(gc_vclock, &r->vclock);
 		vclock_copy(end_vclock, &r->vclock);
-		return;
+		return 0;
 	}
 
 	if (xdir_first_vclock(&r->wal_dir, gc_vclock) < 0)
@@ -134,12 +135,19 @@ recovery_scan(struct recovery *r, struct vclock *end_vclock,
 
 	/* Scan the last xlog to find end vclock. */
 	struct xlog_cursor cursor;
-	if (xdir_open_cursor(&r->wal_dir, vclock_sum(end_vclock), &cursor) != 0)
-		return;
+	if (xdir_open_cursor(&r->wal_dir, vclock_sum(end_vclock), &cursor) != 0) {
+		/*
+		 * FIXME: Why do we ignore errors?!
+		 */
+		return 0;
+	}
+
 	struct xrow_header row;
 	while (xlog_cursor_next(&cursor, &row, true) == 0)
 		vclock_follow_xrow(end_vclock, &row);
+
 	xlog_cursor_close(&cursor, false);
+	return 0;
 }
 
 static int
diff --git a/src/box/recovery.h b/src/box/recovery.h
index 7aa496b14..90fbf1b0a 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -74,7 +74,7 @@ recovery_delete(struct recovery *r);
  * @gc_vclock is set to the oldest vclock available in the
  * WAL directory.
  */
-void
+int
 recovery_scan(struct recovery *r,  struct vclock *end_vclock,
 	      struct vclock *gc_vclock);
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 10/17] recovery: recovery_scan -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 1c7665f87..af7910bdf 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -134,12 +135,19 @@ recovery_scan(struct recovery *r, struct vclock *end_vclock,
>  
>  	/* Scan the last xlog to find end vclock. */
>  	struct xlog_cursor cursor;
> -	if (xdir_open_cursor(&r->wal_dir, vclock_sum(end_vclock), &cursor) != 0)
> -		return;
> +	if (xdir_open_cursor(&r->wal_dir, vclock_sum(end_vclock), &cursor) != 0) {

1. This becomes out of 80 symbols now. I propose to wrap '&cursor'
on the next line.

> +		/*
> +		 * FIXME: Why do we ignore errors?!

2. Since we are talking about this, we also skip
xlog_cursor_next() returning -1 below.

> +		 */
> +		return 0;
> +	}
> +
>  	struct xrow_header row;
>  	while (xlog_cursor_next(&cursor, &row, true) == 0)
>  		vclock_follow_xrow(end_vclock, &row);
> +
>  	xlog_cursor_close(&cursor, false);
> +	return 0;
>  }

[Tarantool-patches] [PATCH 11/17] recovery: recovery_finalize -- don't throw exception

[Cyrill Gorcunov]

Prepare for transition to plain C.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      | 4 +++-
 src/box/recovery.cc | 5 ++---
 src/box/recovery.h  | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index c269a5da8..31c53a443 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2318,7 +2318,9 @@ local_recovery(const struct tt_uuid *instance_uuid,
 		box_listen();
 		box_sync_replication(false);
 	}
-	recovery_finalize(recovery);
+
+	if (recovery_finalize(recovery) != 0)
+		diag_raise();
 
 	/*
 	 * We must enable WAL before finalizing engine recovery,
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index af7910bdf..9ed42590c 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -389,11 +389,10 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 	return 0;
 }
 
-void
+int
 recovery_finalize(struct recovery *r)
 {
-	if (recovery_close_log(r) != 0)
-		diag_raise();
+	return recovery_close_log(r);
 }
 
 
diff --git a/src/box/recovery.h b/src/box/recovery.h
index 90fbf1b0a..a8cfa27d1 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -85,7 +85,7 @@ recovery_follow_local(struct recovery *r, struct xstream *stream,
 void
 recovery_stop_local(struct recovery *r);
 
-void
+int
 recovery_finalize(struct recovery *r);
 
 #if defined(__cplusplus)
-- 
2.20.1

[Tarantool-patches] [PATCH 12/17] recovery: recovery_stop_local -- don't throw exception

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc      | 3 ++-
 src/box/recovery.cc | 5 +++--
 src/box/recovery.h  | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 31c53a443..01ef3318f 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2307,7 +2307,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
 				break;
 			fiber_sleep(0.1);
 		}
-		recovery_stop_local(recovery);
+		if (recovery_stop_local(recovery))
+		    diag_raise();
 		if (recover_remaining_wals(recovery, &wal_stream.base, NULL, true) != 0)
 			diag_raise();
 		/*
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index 9ed42590c..1b5b51b7f 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -581,7 +581,7 @@ recovery_follow_local(struct recovery *r, struct xstream *stream,
 	return 0;
 }
 
-void
+int
 recovery_stop_local(struct recovery *r)
 {
 	if (r->watcher) {
@@ -589,8 +589,9 @@ recovery_stop_local(struct recovery *r)
 		r->watcher = NULL;
 		fiber_cancel(f);
 		if (fiber_join(f) != 0)
-			diag_raise();
+			return -1;
 	}
+	return 0;
 }
 
 /* }}} */
diff --git a/src/box/recovery.h b/src/box/recovery.h
index a8cfa27d1..5e2826fee 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -82,7 +82,7 @@ int
 recovery_follow_local(struct recovery *r, struct xstream *stream,
 		      const char *name, ev_tstamp wal_dir_rescan_delay);
 
-void
+int
 recovery_stop_local(struct recovery *r);
 
 int
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 12/17] recovery: recovery_stop_local -- don't throw exception

[Vladislav Shpilevoy]

Thanks for the patch!

On 28/04/2020 18:11, Cyrill Gorcunov wrote:
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/box.cc      | 3 ++-
>  src/box/recovery.cc | 5 +++--
>  src/box/recovery.h  | 2 +-
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 31c53a443..01ef3318f 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2307,7 +2307,8 @@ local_recovery(const struct tt_uuid *instance_uuid,
>  				break;
>  			fiber_sleep(0.1);
>  		}
> -		recovery_stop_local(recovery);
> +		if (recovery_stop_local(recovery))
> +		    diag_raise();

Something is wrong with the indentation.

Also better do '!= 0' explicitly. Since this is our code style,
and all the other arguments I provided earlier.

[Tarantool-patches] [PATCH 13/17] recovery: cxx to c transition

[Cyrill Gorcunov]

We're ready now to compile the recovery
code in plain C mode.

Part of #3794

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/CMakeLists.txt              |  2 +-
 src/box/{recovery.cc => recovery.c} | 15 +++++++++------
 src/box/recovery.h                  |  8 ++++----
 3 files changed, 14 insertions(+), 11 deletions(-)
 rename src/box/{recovery.cc => recovery.c} (98%)

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index c931ecdfe..34febf026 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -131,7 +131,7 @@ add_library(box STATIC
     user.cc
     authentication.cc
     replication.cc
-    recovery.cc
+    recovery.c
     xstream.cc
     applier.cc
     relay.cc
diff --git a/src/box/recovery.cc b/src/box/recovery.c
similarity index 98%
rename from src/box/recovery.cc
rename to src/box/recovery.c
index 1b5b51b7f..510235ac1 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2010-2016, Tarantool AUTHORS, please see AUTHORS file.
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
  *
  * Redistribution and use in source and binary forms, with or
  * without modification, are permitted provided that the following
@@ -30,11 +30,8 @@
  */
 #include "recovery.h"
 
-#include "small/rlist.h"
-#include "scoped_guard.h"
 #include "trigger.h"
 #include "fiber.h"
-#include "xlog.h"
 #include "xrow.h"
 #include "xstream.h"
 #include "wal.h" /* wal_watcher */
@@ -424,15 +421,21 @@ wal_subscr_wakeup(struct wal_subscr *ws, unsigned int events)
 }
 
 static void
-wal_subscr_dir_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
+wal_subscr_dir_stat_cb(struct ev_loop *loop, struct ev_stat *stat, int revents)
 {
+	(void)revents;
+	(void)loop;
+
 	struct wal_subscr *ws = (struct wal_subscr *)stat->data;
 	wal_subscr_wakeup(ws, WAL_EVENT_ROTATE);
 }
 
 static void
-wal_subscr_file_stat_cb(struct ev_loop *, struct ev_stat *stat, int)
+wal_subscr_file_stat_cb(struct ev_loop *loop, struct ev_stat *stat, int revents)
 {
+	(void)revents;
+	(void)loop;
+
 	struct wal_subscr *ws = (struct wal_subscr *)stat->data;
 	wal_subscr_wakeup(ws, WAL_EVENT_WRITE);
 }
diff --git a/src/box/recovery.h b/src/box/recovery.h
index 5e2826fee..20fd9998b 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -88,10 +88,6 @@ recovery_stop_local(struct recovery *r);
 int
 recovery_finalize(struct recovery *r);
 
-#if defined(__cplusplus)
-} /* extern "C" */
-#endif /* defined(__cplusplus) */
-
 /**
  * Find out if there are new .xlog files since the current
  * vclock, and read them all up.
@@ -106,4 +102,8 @@ int
 recover_remaining_wals(struct recovery *r, struct xstream *stream,
 		       const struct vclock *stop_vclock, bool scan_dir);
 
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
 #endif /* TARANTOOL_RECOVERY_H_INCLUDED */
-- 
2.20.1

[Tarantool-patches] [PATCH 14/17] recovery: drop redundant type_XlogGapError

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/recovery.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/box/recovery.h b/src/box/recovery.h
index 20fd9998b..e8ac3198a 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -40,8 +40,6 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
-extern const struct type_info type_XlogGapError;
-
 struct xrow_header;
 struct xstream;
 
-- 
2.20.1

[Tarantool-patches] [PATCH 15/17] recovery: provide throwable wrappers

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/recovery.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/src/box/recovery.h b/src/box/recovery.h
index e8ac3198a..f085522db 100644
--- a/src/box/recovery.h
+++ b/src/box/recovery.h
@@ -102,6 +102,59 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 
 #if defined(__cplusplus)
 } /* extern "C" */
+
+#include "exception.h"
+
+static inline struct recovery *
+recovery_new_xc(const char *wal_dirname, bool force_recovery,
+		const struct vclock *vclock)
+{
+	struct recovery *r;
+
+	r = recovery_new(wal_dirname, force_recovery, vclock);
+	if (r == NULL)
+		diag_raise();
+	return r;
+}
+
+static inline void
+recovery_scan_xc(struct recovery *r, struct vclock *end_vclock,
+		 struct vclock *gc_vclock)
+{
+	if (recovery_scan(r, end_vclock, gc_vclock))
+		diag_raise();
+}
+
+static inline void
+recover_remaining_wals_xc(struct recovery *r, struct xstream *stream,
+			  const struct vclock *stop_vclock, bool scan_dir)
+{
+	if (recover_remaining_wals(r, stream, stop_vclock, scan_dir))
+		diag_raise();
+}
+
+static inline void
+recovery_follow_local_xc(struct recovery *r, struct xstream *stream,
+			 const char *name, ev_tstamp wal_dir_rescan_delay)
+{
+	if (recovery_follow_local(r, stream, name, wal_dir_rescan_delay))
+		diag_raise();
+}
+
+static inline void
+recovery_stop_local_xc(struct recovery *r)
+{
+	if (recovery_stop_local(r))
+		diag_raise();
+}
+
+static inline void
+recovery_finalize_xc(struct recovery *r)
+{
+	if (recovery_finalize(r))
+		diag_raise();
+}
+
 #endif /* defined(__cplusplus) */
 
 #endif /* TARANTOOL_RECOVERY_H_INCLUDED */
-- 
2.20.1

[Tarantool-patches] [PATCH 16/17] box: use _xc helpers of recovery code

[Cyrill Gorcunov]

To shrink code a bit.

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

diff --git a/src/box/box.cc b/src/box/box.cc
index 01ef3318f..802aa27a2 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2221,11 +2221,9 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	wal_stream_create(&wal_stream);
 
 	struct recovery *recovery;
-	recovery = recovery_new(cfg_gets("wal_dir"),
-				cfg_geti("force_recovery"),
-				checkpoint_vclock);
-	if (recovery == NULL)
-		diag_raise();
+	recovery = recovery_new_xc(cfg_gets("wal_dir"),
+				   cfg_geti("force_recovery"),
+				   checkpoint_vclock);
 
 	/*
 	 * Make sure we report the actual recovery position
@@ -2243,8 +2241,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	 * so we must reflect this in replicaset vclock to
 	 * not attempt to apply these rows twice.
 	 */
-	if (recovery_scan(recovery, &replicaset.vclock, &gc.vclock) != 0)
-		diag_raise();
+	recovery_scan_xc(recovery, &replicaset.vclock, &gc.vclock);
 	say_info("instance vclock %s", vclock_to_string(&replicaset.vclock));
 
 	if (wal_dir_lock >= 0) {
@@ -2285,8 +2282,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	memtx_engine_recover_snapshot_xc(memtx, checkpoint_vclock);
 
 	engine_begin_final_recovery_xc();
-	if (recover_remaining_wals(recovery, &wal_stream.base, NULL, false) != 0)
-		diag_raise();
+	recover_remaining_wals_xc(recovery, &wal_stream.base, NULL, false);
 	engine_end_recovery_xc();
 	/*
 	 * Leave hot standby mode, if any, only after
@@ -2295,22 +2291,19 @@ local_recovery(const struct tt_uuid *instance_uuid,
 	if (wal_dir_lock < 0) {
 		title("hot_standby");
 		say_info("Entering hot standby mode");
-		if (recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
-					  cfg_getd("wal_dir_rescan_delay")) != 0)
-			diag_raise();
+		recovery_follow_local_xc(recovery, &wal_stream.base, "hot_standby",
+					 cfg_getd("wal_dir_rescan_delay"));
 		while (true) {
 			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock)) {
-				recovery_stop_local(recovery);
+				recovery_stop_local_xc(recovery);
 				diag_raise();
 			}
 			if (wal_dir_lock >= 0)
 				break;
 			fiber_sleep(0.1);
 		}
-		if (recovery_stop_local(recovery))
-		    diag_raise();
-		if (recover_remaining_wals(recovery, &wal_stream.base, NULL, true) != 0)
-			diag_raise();
+		recovery_stop_local_xc(recovery);
+		recover_remaining_wals_xc(recovery, &wal_stream.base, NULL, true);
 		/*
 		 * Advance replica set vclock to reflect records
 		 * applied in hot standby mode.
@@ -2320,8 +2313,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
 		box_sync_replication(false);
 	}
 
-	if (recovery_finalize(recovery) != 0)
-		diag_raise();
+	recovery_finalize_xc(recovery);
 
 	/*
 	 * We must enable WAL before finalizing engine recovery,
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 16/17] box: use _xc helpers of recovery code

[Vladislav Shpilevoy]

Thanks for the patch!

Please, don't do that. You literally destroyed your own
changes from the previous commits.

Anyway box.cc and relay.cc will be eventually converted
to C, and the task is getting more complex after this commit.

On 28/04/2020 18:11, Cyrill Gorcunov wrote:
> To shrink code a bit.

You removed 8 lines from box.cc and 9 lines from relay.cc,
but added 53 to recovery.h. And still both box.cc and relay.cc
use != 0 and diag_raise() in other places.

[Tarantool-patches] [PATCH 17/17] relay: use _xc recovery helpers

[Cyrill Gorcunov]

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

diff --git a/src/box/relay.cc b/src/box/relay.cc
index a5bcd9580..b6be3c0bd 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -341,10 +341,8 @@ relay_final_join_f(va_list ap)
 
 	/* Send all WALs until stop_vclock */
 	assert(relay->stream.write != NULL);
-	if (recover_remaining_wals(relay->r, &relay->stream,
-				   &relay->stop_vclock, true) != 0) {
-		diag_raise();
-	}
+	recover_remaining_wals_xc(relay->r, &relay->stream,
+				  &relay->stop_vclock, true);
 	assert(vclock_compare(&relay->r->vclock, &relay->stop_vclock) == 0);
 	return 0;
 }
@@ -363,9 +361,7 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
 		relay_delete(relay);
 	});
 
-	relay->r = recovery_new(cfg_gets("wal_dir"), false, start_vclock);
-	if (relay->r == NULL)
-		diag_raise();
+	relay->r = recovery_new_xc(cfg_gets("wal_dir"), false, start_vclock);
 
 	vclock_copy(&relay->stop_vclock, stop_vclock);
 
@@ -504,10 +500,7 @@ relay_process_wal_event(struct wal_watcher *watcher, unsigned events)
 	}
 	try {
 		bool scan_dir = (events & WAL_EVENT_ROTATE) ? true : false;
-		if (recover_remaining_wals(relay->r, &relay->stream, NULL,
-					   scan_dir) != 0) {
-			diag_raise();
-		}
+		recover_remaining_wals_xc(relay->r, &relay->stream, NULL, scan_dir);
 	} catch (Exception *e) {
 		relay_set_error(relay, e);
 		fiber_cancel(fiber());
@@ -719,9 +712,7 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
 	});
 
 	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
-	relay->r = recovery_new(cfg_gets("wal_dir"), false, replica_clock);
-	if (relay->r == NULL)
-		diag_raise();
+	relay->r = recovery_new_xc(cfg_gets("wal_dir"), false, replica_clock);
 
 	vclock_copy(&relay->tx.vclock, replica_clock);
 	relay->version_id = replica_version_id;
-- 
2.20.1

[Tarantool-patches] [PATCH 01/17] recovery: do not call recovery_stop_local inside recovery_delete

[Cyrill Gorcunov]

From: Georgy Kirichenko <georgy@tarantool.org>

Recovery stop local raises an exception in case of an recovery error
so it is not safe to stop recovery inside recovery delete and guard
inside local_recovery. So call recovery_stop_local manually.

Part of #980, #3794
---
 src/box/box.cc      | 4 +++-
 src/box/recovery.cc | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index c01b08613..1c6fa582c 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2294,8 +2294,10 @@ local_recovery(const struct tt_uuid *instance_uuid,
 		recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
 				      cfg_getd("wal_dir_rescan_delay"));
 		while (true) {
-			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock))
+			if (path_lock(cfg_gets("wal_dir"), &wal_dir_lock)) {
+				recovery_stop_local(recovery);
 				diag_raise();
+			}
 			if (wal_dir_lock >= 0)
 				break;
 			fiber_sleep(0.1);
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index d1a503cfc..1d0e80057 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -216,7 +216,7 @@ recovery_open_log(struct recovery *r, const struct vclock *vclock)
 void
 recovery_delete(struct recovery *r)
 {
-	recovery_stop_local(r);
+	assert(r->watcher == NULL);
 
 	trigger_destroy(&r->on_close_log);
 	xdir_destroy(&r->wal_dir);
-- 
2.20.1