feat(server): SetConfig() for ClusterConfig. #1202

chakaz · 2023-05-10T19:29:20Z

No description provided.

adiholden · 2023-05-11T06:30:31Z

src/server/cluster/cluster_config.cc

-  for (SlotId slot_id = 0; slot_id <= kMaxSlotNum; ++slot_id) {
-    owned_slots_.emplace(slot_id);
+namespace {
+optional<ClusterConfig::Node> GetMasterNodeForSlot(const vector<ClusterConfig::Node>& nodes,


maybe rename to GetMasterNode?

adiholden · 2023-05-11T06:32:42Z

src/server/cluster/cluster_config.cc

+    }
+  }
+
+  CHECK(false) << "No master present in nodes";


we dont want to crash the server if there is problem with the config data. You were thinking to add validation on this on a higher level later?

I'll remove the CHECK() here, and will also add enforcement on SetConfig()

I think its ok to have the CHECK here as an internal system behavior. But the SetConfig should have a return value so if the validations fail we should reply to the set config command with error

adiholden · 2023-05-11T06:34:42Z

src/server/cluster/cluster_config.cc

-  std::shared_lock sl(slots_mu_);
-  return owned_slots_.contains(id);
+bool ClusterConfig::IsMySlot(SlotId id) const {
+  return GetNodeForSlot(id) == nullopt;


We will need to validate that all slots are allocated in the given config, otherwise this logic will fail

Currently (pre-this PR), when the process starts it assumes that it owns all slots. This logic maintains that, but I actually think it should be the other way around, i.e. that the process should start with 0 slots and take on new slots as needed.
This is worth discussing actually, but anyway, as long as config requests are full, we are good. We can enforce that in SetConfig(), like you proposed above.

yes you are right, when the server starts it will start with 0 slots, this is also what I wrote in the design doc. I just created this logic in my PR so that I could check my logic in all unit test with cluster_mode yes as default. But we can fix this now

adiholden · 2023-05-11T06:35:53Z

src/server/cluster/cluster_config.cc

+}
+
+optional<ClusterConfig::Node> ClusterConfig::GetNodeForSlot(SlotId id) const {
+  CHECK(id < config_.size());


adiholden · 2023-05-11T06:38:26Z

src/server/cluster/cluster_config.h

+
+  // This array covers the whole range of possible slots. We keep nullopt for the current instance
+  // ("me"), as we do not need any additional information for it (such as ip and port).
+  std::array<std::optional<Node>, kMaxSlotNum + 1> config_ ABSL_GUARDED_BY(slots_mu_) = {};


I would use different name than config, as this is not the config data we need to save. We will need to save all the config data in SetConfig to be able to reply to the client commands. f.e https://redis.io/commands/cluster-shards/

That's a good point!

adiholden · 2023-05-11T06:51:08Z

src/server/cluster/cluster_config.h

- private:
-  void AddSlots();
+  // Returns nodes that own `id`. Empty if owned by me.
+  std::optional<Node> GetNodeForSlot(SlotId id) const;


You created this function for the MOVE error, right? You will only need tor return the ip and port in this case.
If you save all the config data in SetConfig, as you will need it for later, see my comment on the config member, you can create an array of pair string_view ip and port

A string_view won't work here I'm afraid, as the config may be changed after we return from this method.
And also - we'll need to return the ID and role for CLUSTER SHARDS, so there's no redundant information in returning Node

The return type of this function can be pair<string,string> while the slots array data can be pair<string_view,sting_view>
I believe you will need to save entire config data not in a vector of slot nodes, but as given in SetConfig command, just vector of shards data. Therefor I suggest having the slots array as pair<string_view,sting_view> which will point to the vector of shards data.

I was thinking about keeping only the config_ array (to be renamed to slots_) instead of saving redundant data. Then, upon CLUSTER SHARDS or CLUSTER SLOTS we could build whatever we need (and it's not like the performance of these command matter). I conceive this to be less error prone (fewer points of failure / mismatches, smaller state to maintain).

but you save on the slots_ array only master node

Yes, I'm changing that following your feedback above :)

chakaz · 2023-05-11T19:35:02Z

@adiholden I slightly changed the API following your feedback and some ideas that came into my mind after that.
Specifically, I added an explicit master member to the nodes, and the vector now only represents replicas. This leaves out many error paths (no master, multiple masters) that are now unrepresentable.
It also removes the Role enum, as it's not needed anymore.
PTAL

adiholden · 2023-05-14T07:07:28Z

@adiholden I slightly changed the API following your feedback and some ideas that came into my mind after that. Specifically, I added an explicit master member to the nodes, and the vector now only represents replicas. This leaves out many error paths (no master, multiple masters) that are now unrepresentable. It also removes the Role enum, as it's not needed anymore. PTAL

@chakaz did you push the changes to the branch?

chakaz · 2023-05-14T07:30:25Z

@adiholden I slightly changed the API following your feedback and some ideas that came into my mind after that. Specifically, I added an explicit master member to the nodes, and the vector now only represents replicas. This leaves out many error paths (no master, multiple masters) that are now unrepresentable. It also removes the Role enum, as it's not needed anymore. PTAL

@chakaz did you push the changes to the branch?

Oops, I guess I didn't :)
PTAL, sorry!

adiholden · 2023-05-14T08:00:43Z

src/server/cluster/cluster_config.cc

+  for (const auto& shard : new_config) {
+    for (const auto& slot_range : shard.slot_ranges) {
+      if (slot_range.start > slot_range.end) {
+        return false;


Please add log prints so it will be easier to understand if we have a problem with the config

Oh, that's a great idea!

adiholden · 2023-05-14T08:32:10Z

src/server/cluster/cluster_config.h


 private:
-  void AddSlots();
+  struct SlotOwner {


In this implementation we are using *16384 space to save the data ,instead saving it only once as the config format, and we will need to have a logic to convert from the slot array to the config format again to be able to reply to the CLISTER SHARDS command.

If you would create a member :
std::vector config_;
Which you will set on Set config command and this will be used to reply CLUSTER SHARDS
The SlotOwner struct can be
{
string_view ip;
string_view port;
bool owned_by_me;
}

In this implementation if we have one master and one replica, the data will be saved in the config_ member, the slot array will point to the config data.
In the current implementation we are saving the master and the replica data *16384 times

I guess that it's a matter of what we're optimizing for. The current optimization is aimed for fast "get owners for slot" questions, such as MOVED replies.
If we're looking to save memory, we could consider using shared_ptr as the value of the internal map. It has the downside of using atomics, but I guess it's not terrible since it's only going to take effect during SetConfig() (I think).
But it's true that the current implementation requires more logic to build the reply of CLUSTER SHARDS and CLUSTER SLOTS. Not much more logic, essentially building a dictionary by iterating all entries (that'll actually be simpler to implement if we moved to shared_ptr as we'll be able to compare Nodes). But still, we could avoid compressing ranges.
Do you suggest having 2 members (slots_ and config_, where slots_ will have 16k entries and will point into config_)? Because that will keep the 16k times data duplication, but without it I don't think we can guarantee O(1) computational time for MOVED replies.

Yes we should optimise for "get owners for slot" as it is in the flow of client database queries.
But why does my suggestion guarantee O(1) computational time?

As discussed offline, I kept 2 members and updated the code according to your suggestion. PTAL.

adiholden · 2023-05-14T08:33:00Z

src/server/cluster/cluster_config.h

+
+  // Returns nodes that own `id`. Result will always have the first element set to be the master,
+  // and the following 0 or more elements are the replicas.
+  std::vector<Node> GetNodesForSlot(SlotId id) const;


what is the flow we will need this api for?

CLUSTER SHARDS for example

Removed per the new API

adiholden · 2023-05-14T12:33:33Z

src/server/cluster_config_test.cc

+}
+
+TEST_F(ClusterConfigTest, ConfigSetInvalidEmpty) {
+  // Test that empty config means all slots are owned locally.


Update the comment

adiholden · 2023-05-14T12:37:36Z

src/server/cluster_config_test.cc

+TEST_F(ClusterConfigTest, ConfigEmpty) {
+  // Test that empty-initialization causes none of the slots to be owned locally.
+  for (SlotId i : {0, 1, 10, 100, 1'000, 10'000, 16'000, 0x3FFF}) {
+    EXPECT_FALSE(config_.IsMySlot(i));


please add a test where you check that IsMySlot is true and for some false

That's a good idea!
I changed ClusterConfigTest.ConfigSetMultipleInstances to have that.

feat(server): SetConfig() for ClusterConfig.

feat(server): SetConfig() for ClusterConfig.

85184b2

chakaz requested a review from adiholden May 10, 2023 19:29

chakaz added 2 commits May 10, 2023 23:12

Only save the master node per slot.

0916eb8

format

768e83e

adiholden reviewed May 11, 2023

View reviewed changes

Changes following feedback in PR.

5f4da9b

adiholden reviewed May 14, 2023

View reviewed changes

Fixes according to PR comments.

530cbba

adiholden reviewed May 14, 2023

View reviewed changes

adiholden previously approved these changes May 14, 2023

View reviewed changes

Final changes following PR comments.

1be7e42

chakaz dismissed adiholden’s stale review via 1be7e42 May 14, 2023 12:43

Merge branch 'main' into cluster-config

0740767

adiholden approved these changes May 14, 2023

View reviewed changes

chakaz merged commit d9007ee into main May 14, 2023
2 of 6 checks passed

chakaz deleted the cluster-config branch May 14, 2023 12:54

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

feat(server): SetConfig() for ClusterConfig. (#1202)

e2bb965

feat(server): SetConfig() for ClusterConfig.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): SetConfig() for ClusterConfig. #1202

feat(server): SetConfig() for ClusterConfig. #1202

chakaz commented May 10, 2023

adiholden May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

adiholden May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

adiholden May 11, 2023

adiholden May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

adiholden May 11, 2023

chakaz May 11, 2023

chakaz commented May 11, 2023 •

edited

adiholden commented May 14, 2023

chakaz commented May 14, 2023

adiholden May 14, 2023

chakaz May 14, 2023

adiholden May 14, 2023

chakaz May 14, 2023

adiholden May 14, 2023

chakaz May 14, 2023

adiholden May 14, 2023

chakaz May 14, 2023

chakaz May 14, 2023

adiholden May 14, 2023

adiholden May 14, 2023

chakaz May 14, 2023

feat(server): SetConfig() for ClusterConfig. #1202

feat(server): SetConfig() for ClusterConfig. #1202

Conversation

chakaz commented May 10, 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

chakaz commented May 11, 2023 • edited

adiholden commented May 14, 2023

chakaz commented May 14, 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

chakaz commented May 11, 2023 •

edited