[Tarantool-patches] [PATCH v2 10/11] box: enrich ER_READONLY with new details

[Serge Petrenko]

12.11.2021 02:54, Vladislav Shpilevoy пишет:
> ER_READONLY used not to have any details about the exact reason
> why the instance is read-only. The patch changes that by adding
> new fields into the error which explain why the error happened and
> even help to avoid it for next requests.
>

Thanks for the changes!

Please, find two comments below.

Sorry for coming up late with this one, but I think it'd be good
to report ro reason in box.info. Maybe box.info.ro_reason or
something similar. Only when box.info.ro is true, of course.

Otherwise we help the user only partially. He sees what's wrong when 
receives
an error, but has to check every parameter manually when checks box.info.ro

> ---
>   .../unreleased/gh-5568-readonly-reason.md     |   4 +
>   src/box/box.cc                                |  54 +++-
>   .../gh_5568_read_only_reason_test.lua         | 285 ++++++++++++++++++
>   3 files changed, 335 insertions(+), 8 deletions(-)
>   create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
>   create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
>
> diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
> new file mode 100644
> index 000000000..f3a2db986
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5568-readonly-reason.md
> @@ -0,0 +1,4 @@
> +## feature/core
> +
> +* Error objects with the code `box.error.READONLY` now have additional fields
> +  explaining why the error happened (gh-5568).
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1ed1ce3f8..323982969 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -177,16 +177,54 @@ box_update_ro_summary(void)
>   static int
>   box_check_writable(void)
>   {
> -	if (is_ro_summary) {
> +	if (!is_ro_summary)
> +		return 0;
> +	struct error *e = diag_set(ClientError, ER_READONLY);
> +	struct raft *raft = box_raft();
> +	/*
> +	 * In case of multiple reasons at the same time only one is reported.
> +	 * But the order is important. For example, if the instance has election
> +	 * enabled, for the client it is better to see that it is a 'follower'
> +	 * and who is the leader than just see cfg 'read_only' is true.
> +	 */
> +	if (raft_is_ro(raft)) {
> +		error_set_str(e, "reason", "election");
> +		error_set_str(e, "state", raft_state_str(raft->state));
> +		error_set_uint(e, "term", raft->volatile_term);
> +		uint32_t id = raft->leader;
> +		if (id != REPLICA_ID_NIL) {
> +			error_set_uint(e, "leader_id", id);
> +			struct replica *r = replica_by_id(id);
> +			/*
> +			 * XXX: when the leader is dropped from _cluster, it
> +			 * is not reported to Raft.
> +			 */
> +			if (r != NULL)
> +				error_set_uuid(e, "leader_uuid", &r->uuid);
> +		}
> +	} else if (txn_limbo_is_ro(&txn_limbo)) {
> +		error_set_str(e, "reason", "synchro");
> +		uint32_t id = txn_limbo.owner_id;
> +		error_set_uint(e, "queue_owner_id", id);
> +		error_set_uint(e, "term", raft->volatile_term);

I just noticed, we should report txn_limbo_greatest_term here, probably.

This instance (which received ER_READONLY) is the leader, but hasn't claimed
the limbo yet.
This only makes sense when limbo term is behind raft's one.

> +		struct replica *r = replica_by_id(id);
>   		/*
> -		 * XXX: return a special error when the node is not a leader to
> -		 * reroute to the leader node.
> +		 * XXX: when an instance is deleted from _cluster, its limbo's
> +		 * ownership is not cleared.
>   		 */
> -		diag_set(ClientError, ER_READONLY);
> -		diag_log();
> -		return -1;
> -	}
> -	return 0;
> +		if (r != NULL)
> +			error_set_uuid(e, "queue_owner_uuid", &r->uuid);
> +	} else {
> +		error_set_str(e, "reason", "state");
> +		if (is_ro)
> +			error_set_str(e, "state", "read_only");
> +		else if (is_orphan)
> +			error_set_str(e, "state", "orphan");
> +		else
> +			assert(false);
> +	}
> +	diag_log();
> +	return -1;
>   }
>   
>   static void
>
--
Serge Petrenko