From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D6E66445320 for ; Mon, 6 Jul 2020 17:26:57 +0300 (MSK) Received: by mail-lf1-f65.google.com with SMTP id k17so9918816lfg.3 for ; Mon, 06 Jul 2020 07:26:57 -0700 (PDT) Date: Mon, 6 Jul 2020 17:26:54 +0300 From: Cyrill Gorcunov Message-ID: <20200706142654.GM2256@grain> References: <20200702202141.4821-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200702202141.4821-1-skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [DRAFT v2] replication: track information about replica List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy On Thu, Jul 02, 2020 at 11:21:41PM +0300, Sergey Kaplun wrote: > This is a draft for the patch. > The patch allows to track information about changing relay state. At > every change of relay state timestamp, vclock, new state (and error > message if exists) will be saved at _cluster space. > > The patch adds trigger list at relay, that is invoked when relay changes > its state. The trigger that updates _cluster space is setted when a > replica is registered. > --- > > This is a draft for the patch. Nevertheless I would like to hear as much > criticism as possible. Also it's important for me to hear @kostja's > opinion here. > > Originaly the task splits into the parts [1]: > > 1) Pass extra info from downstreams (box.info.listen) to be saved on > master. Persist it on master. > 2) Persist last status change with its timestamp and vclock on master > for each downstream. > > This patch is the second part. > > As we've discussed with Alexander Turenko, there are several ways how > the first part can be implemented: > > 1) Use additional bytes inside greeting to transport information to > master. The bad thing is that greeting is about authorization, not the > additional information. > > 2) Add a new type of the request (eg IPROTO_INFO). An instance receiving > such a request should provide information about itself. So far, this is > only one field yet. Is it reasonable to add new protocol fields for one > feature? > > Thoughts? > > [1]: https://github.com/tarantool/tarantool/issues/3363#issuecomment-622382549 > > Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-3363-track-replica-status-change > Issue: https://github.com/tarantool/tarantool/issues/3363 Sergey, here is some update on top of this patch I would make. Look I'm still thinking about the code architecture in general so please give me some more time. Still just to share early (completely untested of course :) --- src/box/alter.cc | 51 +++++++++++++++++++++++++++++++-------------------- src/box/box.cc | 6 +++--- src/box/relay.cc | 17 +++++++---------- src/box/schema_def.h | 5 +++++ 4 files changed, 46 insertions(+), 33 deletions(-) Index: tarantool.git/src/box/alter.cc =================================================================== --- tarantool.git.orig/src/box/alter.cc +++ tarantool.git/src/box/alter.cc @@ -4172,51 +4172,62 @@ relay_on_state_change(struct trigger *tr { struct relay *relay = (struct relay *)event; (void)trigger; + if (relay_get_state(relay) == RELAY_OFF) return 0; + struct replica *replica = relay_replica(relay); const struct tt_uuid *uuid = &replica->uuid; + assert(replica_by_uuid(uuid) != NULL); assert(replica->id != REPLICA_ID_NIL); + struct credentials *orig_credentials = effective_user(); fiber_set_user(fiber(), &admin_credentials); - int rc; - if ((rc = boxk(IPROTO_UPDATE, BOX_CLUSTER_ID, "[%u][" - "[%s%u%lf]" /* last row time */ - "[%s%u%s]" /* vclock */ - "[%s%u%s]" /* relay state */ - "]", - (unsigned) replica->id, - "=", 3, relay_last_row_time(relay), - "=", 4, vclock_to_string(relay_vclock(relay)), - "=", 5, relay_get_state_str(relay) - )) != 0) { + + const char *vclock_str = vclock_to_string(relay_vclock(relay)); + const char *state_str = relay_get_state_str(relay); + double timestamp = relay_last_row_time(relay); + + int rc = boxk(IPROTO_UPDATE, BOX_CLUSTER_ID, "[%u][" + "[%s%u%lf]" + "[%s%u%s]" + "[%s%u%s]" + "]", + (unsigned) replica->id, + "=", BOX_CLUSTER_FIELD_LAST_ROW_TIME, timestamp, + "=", BOX_CLUSTER_FIELD_VCLOCK, vclock_str, + "=", BOX_CLUSTER_FIELD_RELAY_STATE, state_str); + if (rc != 0) goto restore_cred; - } + + switch (relay_get_state(relay)) { case RELAY_STOPPED: { - struct error *e = - diag_last_error(relay_get_diag(relay)); - if (e != NULL) + struct *diag = relay_get_diag(relay); + struct error *e = diag_last_error(diag); + if (e != NULL) { rc = boxk(IPROTO_UPDATE, BOX_CLUSTER_ID, "[%u][[%s%u%s]]", (unsigned) replica->id, - "=", 6, e->errmsg); + "=", BOX_CLUSTER_FIELD_RELAY_ERR, + e->errmsg); + } break; } case RELAY_FOLLOW: rc = boxk(IPROTO_UPDATE, BOX_CLUSTER_ID, "[%u][[%s%uNIL]]", - (unsigned) replica->id, "=", 6); + (unsigned) replica->id, + "=", BOX_CLUSTER_FIELD_RELAY_ERR); break; default: unreachable(); } + restore_cred: fiber_set_user(fiber(), orig_credentials); - if (rc != 0) - return -1; - return 0; + return rc; } static inline void Index: tarantool.git/src/box/box.cc =================================================================== --- tarantool.git.orig/src/box/box.cc +++ tarantool.git/src/box/box.cc @@ -1476,8 +1476,8 @@ box_session_push(const char *data, const return rc; } -static inline void -box_register_replica(uint32_t id, const struct tt_uuid *uuid) +static void +box_register_replica_xc(uint32_t id, const struct tt_uuid *uuid) { if (boxk(IPROTO_INSERT, BOX_CLUSTER_ID, "[" "%u" /* replica id */ @@ -1524,7 +1524,7 @@ box_on_join(const tt_uuid *instance_uuid break; replica_id++; } - box_register_replica(replica_id, instance_uuid); + box_register_replica_xc(replica_id, instance_uuid); } void Index: tarantool.git/src/box/relay.cc =================================================================== --- tarantool.git.orig/src/box/relay.cc +++ tarantool.git/src/box/relay.cc @@ -153,16 +153,13 @@ struct relay { const char * relay_get_state_str(const struct relay *relay) { - switch(relay->state) { - case RELAY_OFF: - return "off"; - case RELAY_FOLLOW: - return "follow"; - case RELAY_STOPPED: - return "stopped"; - default: - return ""; - } + static const char *st[] = { + [RELAY_OFF] = "off", + [RELAY_FOLLOW] = "follow", + [RELAY_STOPPED] = "stopped", + }; + return relay->state < lengthof(st) ? + st[relay->state] : "unknown"; } static inline void Index: tarantool.git/src/box/schema_def.h =================================================================== --- tarantool.git.orig/src/box/schema_def.h +++ tarantool.git/src/box/schema_def.h @@ -206,6 +206,11 @@ enum { enum { BOX_CLUSTER_FIELD_ID = 0, BOX_CLUSTER_FIELD_UUID = 1, + BOX_CLUSTER_FIELD_HOST_PORT = 2, + BOX_CLUSTER_FIELD_RELAY_TIMESTAMP = 3, + BOX_CLUSTER_FIELD_RELAY_VCLOCK = 4, + BOX_CLUSTER_FIELD_RELAY_STATE = 5, + BOX_CLUSTER_FIELD_RELAY_ERR = 6, }; /** _truncate fields. */