feat(cluster): check command keys ownership #1194

adiholden · 2023-05-08T08:23:42Z

If cluster mode is enabled

On server start add all slots to my ownership (this is temporary untill we handle the config command)
On command flow , check the slot of all keys ,if key is not in slot ownership return MOVED

chakaz · 2023-05-08T08:31:49Z

src/server/cluster/cluster_data.h

+class ClusterData {
+ public:
+  ClusterData();
+  static uint16_t keyHashSlot(std::string_view key);


I'd add a typedef for this uint16_t instead of repeating it, maybe using SlotId = uint16_t?

chakaz · 2023-05-08T08:32:46Z

src/server/cluster/cluster_data.h

+  ClusterData();
+  static uint16_t keyHashSlot(std::string_view key);
+
+  bool IsKeyInMySlot(std::string_view key);


Maybe IsKeyInMySlots() (plural)?
Or IsKeyOwned()?

chakaz · 2023-05-08T08:33:44Z

src/server/cluster/cluster_data.h

+  bool AddSlots();
+
+  util::SharedMutex slots_mu_;
+  std::unordered_set<uint16_t> owned_slots_;


It's mostly better to use absl::flat_hash_set instead of unordered_set

chakaz · 2023-05-08T08:34:27Z

src/server/cluster/cluster_data.h

+
+  util::SharedMutex slots_mu_;
+  std::unordered_set<uint16_t> owned_slots_;
+  static constexpr uint16_t kMaxSlotNum = 0x3FFF;


I'd keep this implementation detail in the .cc file

chakaz · 2023-05-08T08:37:15Z

src/server/cluster/cluster_data.cc

+
+bool cluster_enabled = false;
+
+// If the key contains the {...} pattern, return only the part between { and }


Can you move this documentation to the .h file?

chakaz · 2023-05-08T08:57:45Z

src/server/cluster/cluster_data.cc

+  slots_mu_.lock_shared();
+  size_t count = owned_slots_.count(slot_id);
+  slots_mu_.unlock_shared();


While count() shouldn't throw any exceptions, it's generally safer to use scoped guards like you did above.
Here, the unlocking really doesn't help us as all there's left outside is the return and a comparison, so maybe just create a guard above?
If not, I'd define a scoped guard with a narrower scope, like:

size_t count; { lock_guard gu(slots_mu_); count = owned_slots_.count(slot_id); } return count > 0;

I am calling lock_shared and there is not shard_lock_gaurd in std ,right?

I believe you're looking for std::shared_lock: https://en.cppreference.com/w/cpp/thread/shared_lock :)

chakaz · 2023-05-08T08:58:40Z

src/server/cluster/cluster_data.h

+class ClusterData {
+ public:
+  ClusterData();
+  static uint16_t keyHashSlot(std::string_view key);


Maybe call it KeySlot() instead, as hashing is really an implementation detail?

chakaz · 2023-05-08T09:57:55Z

src/server/cluster_data_test.cc

+  ClusterDataTest() {
+  }


We can remove this constructor if it's empty

chakaz · 2023-05-08T10:30:16Z

src/server/main_service.cc

+    string_view key = ArgS(args, i);
+    if (!server_family_.cluster_data()->IsKeyInMySlot(key)) {
+      VLOG(1) << "Key " << key << " is not owned by server";
+      (*dfly_cntx)->SendError("MOVED");  // TODO add more info to moved error.


Let's think of the case in which a command was issued on multiple keys, and some are owned by this instance, but others are not.
In this case, I don't think that the proper response is MOVED, because there's no way to issue that command no matter which instance receives it.

Actually, any command which runs on multiple instances can't be issued, and we also can't return MOVED for it (we won't be able to redirect the response to any specific instance).
As such, perhaps we should change the IsKeyInMySlot() to be GetSlotForKey(key), which returns a slot id.
Then we can determine the response:

If 1 instance, and it is the current instance: handle it as usual

If 1 instance but it's not the current one: return MOVED

If it's >1: return an error

actually, I will block requests on different slots, as redis does.
So I will check first if keys belong to different slots and than if this slot belongs to me

I believe that's what I proposed, just ordered differently :)

chakaz · 2023-05-08T10:32:26Z

src/server/main_service.h

@@ -118,6 +118,8 @@ class Service : public facade::ServiceInterface {
  // Return false if command is invalid and reply with error.
  bool VerifyCommand(const CommandId* cid, CmdArgList args, ConnectionContext* cntx);

+  bool CheckKeysOwnership(const CommandId* cid, CmdArgList args, ConnectionContext* dfly_cntx);


Please add some documentation. Please include that this function returns true if all keys are owned by this instance, false otherwise. If false is returned, an error has already been send to the client.

chakaz · 2023-05-08T10:51:02Z

src/server/cluster/cluster_data.h

+
+namespace dfly {
+
+class ClusterData {


This isn't really data, right? As in, this class is not responsible for any data nor data handling.
Maybe we can call it ClusterSlots? Or ClusterKeying?

I think this class will be used to hold all the cluster configuration, right now its only slots data

So how about ClusterConfiguration?
I personally think that "data" is too generic, but obviously your call.

ok will change

src/server/cluster_data_test.cc

chakaz · 2023-05-08T12:43:46Z

src/server/cluster/cluster_data.cc

+  // TODO update logic acording to config
+  // currently add all slots to owned slots
+  std::lock_guard lk{slots_mu_};
+  for (uint16_t slot_id = 0; slot_id <= kMaxSlotNum; ++slot_id) {


It's not ideal to consume so much RAM for so little information. If there are 3 nodes in the cluster, each will have a map with >5000 entries. Even if we moved to absl::flat_hash_set the overhead would still be quite big.
However, since we will need to store all slots (to be able to properly redirect), we'll end up storing 16k entries, each holding multiple strings (IDs and roles).
We could use an interval map here (like Boost's), but perhaps it's not that bad to consume <1MB for cluster related metadata.
We could also do a flat_hash_map<uint16_t, std::shared_ptr<Instance>> to have a single instance data allocated.
Just an idea, do whatever you want with it.

I am a little bit confused. first you say that using absl::flat_hash_set will be overhead , but than you suggest to use flat_hash_map<uint16_t, std::shared_ptr>?

Currently I dont have instance data structure. We will need it for the host in the MOVED error, but lets add it once the config is done. Ok?

Sorry :)
I tried to say that any "regular" map will have the overhead of not "compressing" ranges to a single entry (like saying slots [0, 100) belong to instance X).
I realize (and Roman says so as well) that using an interval map is not worth it, so maybe instead of copying the value (in this case the hosts, ips and roles of the instances) 16k times, we'll hold a single shared_ptr for each instance and point to it form the map.
Then again, I guess it's not that bad and Roman thinks we can do with a // TODO so your call :)

romange · 2023-05-08T12:58:29Z

I think interval tree is an overkill here just bevause data is bounded as you mentioned and has less than 0.1% overhead. shared_ptr or a todo could both fit here at this point

…

On Mon, May 8, 2023, 15:44 Chaka ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/server/cluster/cluster_data.cc <#1194 (comment)> : > +} + +uint16_t ClusterData::keyHashSlot(std::string_view key) { + std::string_view tag = KeyTag(key); + return crc16(tag.data(), tag.length()) & kMaxSlotNum; +} + +ClusterData::ClusterData() { + AddSlots(); +} + +bool ClusterData::AddSlots() { + // TODO update logic acording to config + // currently add all slots to owned slots + std::lock_guard lk{slots_mu_}; + for (uint16_t slot_id = 0; slot_id <= kMaxSlotNum; ++slot_id) { It's not ideal to consume so much RAM for so little information. If there are 3 nodes in the cluster, each will have a map with >5000 entries. Even if we moved to absl::flat_hash_set the overhead would still be quite big. However, since we will need to store all slots (to be able to properly redirect), we'll end up storing 16k entries, each holding multiple strings (IDs and roles). We could use an interval map here (like Boost's <https://www.boost.org/doc/libs/1_59_0/libs/icl/doc/html/index.html#boost_icl.introduction>), but perhaps it's not that bad to consume <1MB for cluster related metadata. We could also do a flat_hash_map<uint16_t, std::shared_ptr<Instance>> to have a single instance data allocated. Just an idea, do whatever you want with it. — Reply to this email directly, view it on GitHub <#1194 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4BFCGRJYCAWXPBESUIERLXFDTBDANCNFSM6AAAAAAXZTU62U> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

chakaz · 2023-05-08T19:40:13Z

src/server/cluster/cluster_data.cc

+  // TODO update logic acording to config
+  // currently add all slots to owned slots
+  std::lock_guard lk{slots_mu_};
+  for (uint16_t slot_id = 0; slot_id <= kMaxSlotNum; ++slot_id) {


Sorry :)
I tried to say that any "regular" map will have the overhead of not "compressing" ranges to a single entry (like saying slots [0, 100) belong to instance X).
I realize (and Roman says so as well) that using an interval map is not worth it, so maybe instead of copying the value (in this case the hosts, ips and roles of the instances) 16k times, we'll hold a single shared_ptr for each instance and point to it form the map.
Then again, I guess it's not that bad and Roman thinks we can do with a // TODO so your call :)

chakaz · 2023-05-09T10:08:25Z

src/server/cluster/cluster_config.h

+// If the key contains the {...} pattern, return only the part between { and }
+std::string_view KeyTag(std::string_view key);
+
+extern bool cluster_enabled;


Have you decided against exporting these from within ClusterConfig?
(ideally cluster_enabled will be wrapped in a static method)

src/server/cluster/cluster_data.cc

chakaz · 2023-05-09T11:07:00Z

src/server/cluster/cluster_data.cc

+  // currently add all slots to owned slots
+  std::lock_guard lk{slots_mu_};
+  for (uint16_t slot_id = 0; slot_id <= kMaxSlotNum; ++slot_id) {
+    owned_slots_.emplace(slot_id);


Re/ safety, see a few examples here: https://stackoverflow.com/questions/10890653/why-would-i-ever-use-push-back-instead-of-emplace-back

The actual difference between these two statements is that the more powerful emplace_back will call any type of constructor out there, whereas the more cautious push_back will call only constructors that are implicit

With a good example:

std::vector<std::unique_ptr<T>> v; T a; v.emplace_back(std::addressof(a)); // compiles v.push_back(std::addressof(a)); // fails to compile

chakaz · 2023-05-09T11:07:59Z

src/server/main_service.cc

+    string_view key = ArgS(args, i);
+    if (!server_family_.cluster_data()->IsKeyInMySlot(key)) {
+      VLOG(1) << "Key " << key << " is not owned by server";
+      (*dfly_cntx)->SendError("MOVED");  // TODO add more info to moved error.


I believe that's what I proposed, just ordered differently :)

src/server/cluster/cluster_config.cc

chakaz · 2023-05-10T05:56:39Z

src/server/main_service.cc

+
+  const auto& key_index = *key_index_res;
+  SlotId keys_slot;
+  bool crossslot = false;


nit: cross_slot?

chakaz · 2023-05-10T06:01:14Z

src/server/cluster/cluster_config.cc

+  size_t count = owned_slots_.count(id);
+  return count > 0;


Maybe return owned_slots.contains(id); ?

Signed-off-by: adi_holden <adi@dragonflydb.io>

dranikpg · 2023-05-12T20:24:47Z

src/server/main_service.cc

+  }
+
+  const auto& key_index = *key_index_res;
+  SlotId keys_slot;


@adiholden

I got this warning on compilation and was about to ignore it.

../src/server/main_service.cc: In member function ‘bool dfly::Service::CheckKeysOwnership(const dfly::CommandId*, facade::CmdArgList, dfly::ConnectionContext*)’: ../src/server/main_service.cc:625:49: warning: ‘keys_slot’ may be used uninitialized in this function [-Wmaybe-uninitialized] 625 | if (!server_family_.cluster_config()->IsMySlot(keys_slot)) {

But it seems that slot_id can actually be non initialized. Because even if cid->first_key_pos() > 0, the command itself can have 0 keys if it uses variadic keys. For example: EVAL {script} 0.

thanks @dranikpg

* feat(cluster): check command keys ownership on cluster mode Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested a review from chakaz May 8, 2023 08:23

chakaz reviewed May 8, 2023

View reviewed changes

src/server/cluster_data_test.cc Outdated Show resolved Hide resolved

chakaz reviewed May 8, 2023

View reviewed changes

adiholden requested a review from chakaz May 9, 2023 09:34

chakaz previously approved these changes May 10, 2023

View reviewed changes

adiholden added 7 commits May 10, 2023 09:37

feat(cluster): check command keys ownership

936cc5e

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix PR

f6114dc

Signed-off-by: adi_holden <adi@dragonflydb.io>

change cluster enabled to be static member of the cluster config class

3011e53

Signed-off-by: adi_holden <adi@dragonflydb.io>

change KeyTag to be static member function

0cc4c93

Signed-off-by: adi_holden <adi@dragonflydb.io>

Add slots return void

27c5b35

Signed-off-by: adi_holden <adi@dragonflydb.io>

imporve key tag function

30e1f42

Signed-off-by: adi_holden <adi@dragonflydb.io>

empry

23f27c3

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden dismissed chakaz’s stale review via 23f27c3 May 10, 2023 06:38

adiholden force-pushed the cluster_cmd_key_owner branch from 7f93d36 to 23f27c3 Compare May 10, 2023 06:38

adiholden added 2 commits May 10, 2023 09:40

rename

d1c6298

Signed-off-by: adi_holden <adi@dragonflydb.io>

contains

e9a92a1

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested a review from chakaz May 10, 2023 08:33

chakaz approved these changes May 10, 2023

View reviewed changes

adiholden merged commit 577472e into main May 10, 2023
6 checks passed

dranikpg reviewed May 12, 2023

View reviewed changes

romange deleted the cluster_cmd_key_owner branch May 13, 2023 11:25

romange pushed a commit that referenced this pull request Jun 1, 2023

feat(cluster): check command keys ownership (#1194)

7744120

* feat(cluster): check command keys ownership on cluster mode Signed-off-by: adi_holden <adi@dragonflydb.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cluster): check command keys ownership #1194

feat(cluster): check command keys ownership #1194

adiholden commented May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

adiholden May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

chakaz May 8, 2023

adiholden May 9, 2023

chakaz May 9, 2023

chakaz May 8, 2023

chakaz May 8, 2023

adiholden May 8, 2023

chakaz May 8, 2023

adiholden May 9, 2023

chakaz May 8, 2023

adiholden May 8, 2023

chakaz May 8, 2023

romange commented May 8, 2023 via email

chakaz May 8, 2023

chakaz May 9, 2023

chakaz May 9, 2023

chakaz May 9, 2023

chakaz May 10, 2023

chakaz May 10, 2023

dranikpg May 12, 2023

adiholden May 14, 2023


		bool cluster_enabled = false;

		// If the key contains the {...} pattern, return only the part between { and }

feat(cluster): check command keys ownership #1194

feat(cluster): check command keys ownership #1194

Conversation

adiholden commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange commented May 8, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment