From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Konstantin Osipov <kostja@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [PATCH 4/6] txn: do not require space id for nop requests Date: Thu, 28 Jun 2018 12:13:16 +0300 [thread overview] Message-ID: <20180628091316.hyytupw22x2pvpcy@esperanza> (raw) In-Reply-To: <20180627174509.GH28358@chai> On Wed, Jun 27, 2018 at 08:45:09PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]: > > Currently, IPROTO_NOP can only be generated by a before_replace trigger, > > when it returns the old tuple thus turning the original operation into a > > NOP. In such a case we know the space id and we write it to the request > > body. This allows us to dispatch NOP requests via DML route. > > > > As a part of replica local spaces feature, we will substitute requests > > operating on local spaces with NOP in relay in order to promote vclock > > on replicas without actual data modification. Since space_id is stored > > in request body, sending it to replicas would mean decoding the request > > body in relay, which is an overkill. To avoid that, let's separate NOP > > and DML paths and remove space_id from NOP requests. > > LGTM, a few comments below. > > > @@ -277,6 +277,11 @@ apply_row(struct xstream *stream, struct xrow_header *row) > > (void) stream; > > struct request request; > > xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type)); > > + if (request.type == IPROTO_NOP) { > > + if (txn_commit_nop(&request) != 0) > > + diag_raise(); > > + return; > > + } > > txn_commit_nop() -> box_process_nop() or process_nop(). But txn_commit_nop() lives in txn.c. That's why I called it like that. > > > @@ -50,7 +50,6 @@ request_create_from_tuple(struct request *request, struct space *space, > > struct tuple *old_tuple, struct tuple *new_tuple) > > { > > memset(request, 0, sizeof(*request)); > > - request->space_id = space->def->id; > > > > if (old_tuple == new_tuple) { > > /* > > @@ -61,6 +60,7 @@ request_create_from_tuple(struct request *request, struct space *space, > > return 0; > > } > > > > + request->space_id = space->def->id; > > Please add a comment > /* > * Space pointer may be zero in case of NOP, in which case this > * line is not reached. > */ OK. > > You changed a lot of code making it more NOP friendly, for example > txn_begin_in_engine() now works even if there is already a > transaction which has a NOP statement. This seems like a bug fix > with no test. This is not a bug fix: before this patch there couldn't be a transaction without engine, because NOP requests also had engine set as they needed a space. > Could you please add a test? The code is already covered by tests. See replication/before_replace, box/before_replace.
next prev parent reply other threads:[~2018-06-28 9:13 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov 2018-06-13 16:10 ` [PATCH 1/6] txn: remove unused C++ wrappers Vladimir Davydov 2018-06-27 17:08 ` Konstantin Osipov 2018-06-13 16:10 ` [PATCH 2/6] xrow: fix ret code on decode failure Vladimir Davydov 2018-06-27 17:29 ` Konstantin Osipov 2018-06-13 16:10 ` [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code Vladimir Davydov 2018-06-27 17:48 ` Konstantin Osipov 2018-06-13 16:10 ` [PATCH 4/6] txn: do not require space id for nop requests Vladimir Davydov 2018-06-27 17:45 ` Konstantin Osipov 2018-06-28 9:13 ` Vladimir Davydov [this message] 2018-06-28 10:23 ` Konstantin Osipov 2018-06-28 10:35 ` Vladimir Davydov 2018-06-28 10:54 ` Konstantin Osipov 2018-06-28 11:10 ` Vladimir Davydov 2018-06-13 16:10 ` [PATCH 5/6] xrow: make NOP requests bodiless Vladimir Davydov 2018-06-27 17:49 ` Konstantin Osipov 2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov 2018-06-13 21:26 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-27 18:24 ` Konstantin Osipov 2018-06-27 18:27 ` Konstantin Osipov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180628091316.hyytupw22x2pvpcy@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [PATCH 4/6] txn: do not require space id for nop requests' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox