feat(server): Implement DFLY CLUSTER CONFIG command.
#1223
Conversation
src/server/dflycmd.cc
Outdated
| SinkReplyBuilder* rb = cntx->reply_builder(); | ||
|
|
||
| if (args.size() != 3) { | ||
| return rb->SendError(kSyntaxErr); |
There was a problem hiding this comment.
Check out this error type - WrongNumArgsError
src/server/dflycmd.cc
Outdated
| optional<JsonType> json = JsonFromString(json_str); | ||
| if (!json.has_value()) { | ||
| LOG(WARNING) << kInvalidConfigPrefix << "Can't parse JSON " << json_str; | ||
| return rb->SendError(kSyntaxErr); |
There was a problem hiding this comment.
better return more details in the error here
rb->SendError(kInvalidConfigPrefix, kSyntaxErrType)
src/server/dflycmd.cc
Outdated
| return rb->SendError(kSyntaxErr); | ||
| } | ||
|
|
||
| optional<ClusterConfig::ClusterShards> config = BuildClusterConfigFromJson(json.value()); |
There was a problem hiding this comment.
To me it makes sense that the function BuildClusterConfigFromJson impl will be inside the ClusterConfig class
i.e change SetConfig to get json object, and all the logic to parse the json will be inside ClusterConfig
There was a problem hiding this comment.
I agree. I actually have thought about this before making the change and (originally) thought that it made sense within dflycmd.cc, but following your comment I'm convinced that it's probably better to be part of ClusterConfig' API.
So as you requested, I moved it.
src/server/dflycmd_test.cc
Outdated
| @@ -0,0 +1,374 @@ | |||
| // Copyright 2022, DragonflyDB authors. All rights reserved. | |||
| class DflyFamilyTest : public BaseFamilyTest { | ||
| public: | ||
| DflyFamilyTest() { | ||
| auto* flag = absl::FindCommandLineFlag("cluster_mode"); |
There was a problem hiding this comment.
Because you are setting the cluster mode flag here, I would call this tests DflyClusterTest , cause we might want to add to this file more test which are not under cluster mode
| @@ -61,7 +64,9 @@ class ClusterConfig { | |||
| // Returns true if `new_config` is valid and internal state was changed. Returns false and changes | |||
| // nothing otherwise. | |||
| bool SetConfig(const ClusterShards& new_config); | |||
There was a problem hiding this comment.
Generally, I'd agree. I just tried to move it, but it would then require testing solely by passing json, which is more cumbersome. Also, I guess it's a nicer API to support both, although I agree that if it's not in use there's no point in keeping it.
Again, I'd love to keep it if only to make testing this class easier, OK?
* fix: Lock before accessing slots_ * Implement `DFLY CLUSTER CONFIG` command. * Move JSON parsing logic to ClusterConfig * clang-tidy * Rename test fixture class
No description provided.