From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org, gorcunov@gmail.com Subject: Re: [Tarantool-patches] [PATCH 1/1] replication: check rs uuid on subscribe process Date: Wed, 2 Jun 2021 09:36:23 +0300 [thread overview] Message-ID: <a3fc09d0-fb6f-8d96-1e47-c5e83a85f573@tarantool.org> (raw) In-Reply-To: <2da9c4ac-a10c-9cbd-ec4b-ef35aecd0ea0@tarantool.org> 02.06.2021 00:34, Vladislav Shpilevoy пишет: > Hi! Thanks for the review! Thanks for the fixes! > >>> The UUID ignorance on subscribe decode was introduced here: >>> https://github.com/tarantool/tarantool/commit/7f8cbde3555084ad6c41f137aec4faba4648c705#diff-fc276b44b551b4eac3431c9433d4bc881790ddd7df76226d7579f80da7798f6e >>> >>> And I don't understand why. Maybe I miss something? The tests have >>> passed. Sergey, do you remember why was it needed really? >>> >>> Replicaset UUID mismatch definitely means the node can't connect. >>> It is not related to whether it is anonymous or not. Because it >>> has nothing to do with _cluster. >> Hi! Thanks for the patch! >> >> That change was meant to help anonymous replicas fetch data from >> multiple clusters. There was such an idea back then. >> It never got implemented though, and I doubt it will. > Wow, I am not sure how it is supposed to work at all. Starting from > the problem that in different RS you have conflicting replica_id, and > you won't be able to keep track of the changes properly. You simply can't > pack the same replica_id from 2 different replicasets into one replica_id > in your own journal. > > It does not matter if you are anon or not - the changes in the given > replicasets are generated by non-anon nodes and their replica IDs will > clash. I guess you could assign the same replica_id to all the changes coming from one cluster. Just assign lsns in the order of arrival, or something. Or you could maintain a vclock per each cluster, assign the same replica_id to each change coming from this cluster, and assign vclock_sum as the change's lsn. > >> So the idea was that replica should check the replicaset UUID itself. >> It doesn't work now obviously. And looks like it hadn't worked before >> you introduced the ER_TOO_EARLY_SUBSCRIBE error. >> The test for checking uuid on replica side didn't catch this problem >> because the replica was already registered on master in it. >> >> Long story short, I'm ok with this change, but now you should remove >> unnecessary replicaset uuid checks from replica side (in applier_subscribe). > If it is not critical for you, I would leave as many checks as possible > on both sides. It has nothing to do with perf but protects from unexpected > situations like this one. Let me know if you want it deleted anyway. Ok, I'm not against it. >> And looks like replication/gh-3704-misc-replica-checks-cluster-id test is >> obsolete now. > Yes, looks like so. I deleted it on the branch. Sorry, one more nit. I failed to notice this earlier, but there's an obsolete comment in box_process_subscribe(). It goes like "Tarantool > 2.1.1 master doesn't check that replica has the same cluster uuid". LGTM, once you delete the comment. -- Serge Petrenko
next prev parent reply other threads:[~2021-06-02 6:36 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-28 20:35 Vladislav Shpilevoy via Tarantool-patches 2021-05-28 22:06 ` Cyrill Gorcunov via Tarantool-patches 2021-05-28 22:20 ` Vladislav Shpilevoy via Tarantool-patches 2021-05-28 22:30 ` Cyrill Gorcunov via Tarantool-patches 2021-06-01 7:52 ` Cyrill Gorcunov via Tarantool-patches 2021-06-01 8:29 ` Serge Petrenko via Tarantool-patches 2021-06-01 21:34 ` Vladislav Shpilevoy via Tarantool-patches 2021-06-02 6:36 ` Serge Petrenko via Tarantool-patches [this message] 2021-06-02 20:16 ` Vladislav Shpilevoy via Tarantool-patches 2021-06-03 7:14 ` Serge Petrenko via Tarantool-patches 2021-06-03 7:18 ` Serge Petrenko via Tarantool-patches
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=a3fc09d0-fb6f-8d96-1e47-c5e83a85f573@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 1/1] replication: check rs uuid on subscribe process' \ /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