feat: Use const ConnectionContext in VerifyCommand #1633

dranikpg · 2023-08-03T10:20:38Z

Its an anti-pattern to use a mutable ConnectionContext pointer for both connection state queries and error replies. As a result we have a lot of excessive mutability and we pass it everywhere around. This might have not been an issue so far, but it becomes hard to manage with squashed command execution.

Its needed for squashed pipeline execution. Pipeline commands can make state changes (context & global), so command verification needs to happen immediately before execution. With squashed execution, this verification will take place from multiple threads -> possible trouble if not carefully done

dranikpg · 2023-08-03T10:26:37Z

src/facade/facade_types.h

+struct ErrorReply {
+  explicit ErrorReply(std::string msg, std::string_view kind = {})
+      : message{move(msg)}, kind{kind} {
+  }
+  explicit ErrorReply(std::string_view msg, std::string_view kind = {}) : message{msg}, kind{kind} {
+  }
+  explicit ErrorReply(const char* msg, std::string_view kind = {})
+      : message{std::string_view{msg}}, kind{kind} {
+  }
+  explicit ErrorReply(OpStatus status) : message{}, kind{}, status{status} {
+  }
+
+  std::variant<std::string, std::string_view> message;
+  std::string_view kind;
+  std::optional<OpStatus> status{std::nullopt};
+};
+


I don't feel strong about having such a heavy type inside facade_types (which is included almost everywhere, but not sure where else to put it)

I know its not used everywhere where it fits in, I'll have more iterations when I check other parts

The const char msg* overload is to avoid ambiguity (const char* converts to both string and string_view implicitly)

Maybe in order to reduce ambiguity, the std::string c'tor could accept a std::string&& instead? (I imagine this will allow removing the const char* one, which is confusing).
If for some reason that doesn't work, I'd write that in a comment.

🤷🏻

Added a comment instead

chakaz

Please see some comments, mostly nits

chakaz · 2023-08-03T11:00:16Z

src/facade/facade_types.h

+struct ErrorReply {
+  explicit ErrorReply(std::string msg, std::string_view kind = {})
+      : message{move(msg)}, kind{kind} {
+  }
+  explicit ErrorReply(std::string_view msg, std::string_view kind = {}) : message{msg}, kind{kind} {
+  }
+  explicit ErrorReply(const char* msg, std::string_view kind = {})
+      : message{std::string_view{msg}}, kind{kind} {
+  }
+  explicit ErrorReply(OpStatus status) : message{}, kind{}, status{status} {
+  }
+
+  std::variant<std::string, std::string_view> message;
+  std::string_view kind;
+  std::optional<OpStatus> status{std::nullopt};
+};
+


Maybe in order to reduce ambiguity, the std::string c'tor could accept a std::string&& instead? (I imagine this will allow removing the const char* one, which is confusing).
If for some reason that doesn't work, I'd write that in a comment.

chakaz · 2023-08-03T11:01:40Z

src/facade/facade_types.h

+  explicit ErrorReply(OpStatus status) : message{}, kind{}, status{status} {
+  }
+
+  std::variant<std::string, std::string_view> message;


Maybe we can have message of type string_view, and some other member (like private message_memory_) which message may or may not point into?
It should make using this struct easier

I remember you don't like using variants for storage at all, we already had this case once.
But I like to avoid self referential structs - it requires defining move/copy constructors and operators

I am not worried about performance implications of using std::string in case we return errors.

This is why I use string_view whenever its possible, in other cases you construct a string with StrCat and others anyways

I'll check once again for performance degradation, but it shouldn't be the case

Btw I once thought of supporting an absl::Span<absl::Alphanumeric> interface to write directly into the conn buffer without the intermediary string, but its just a random comment in this context 🙂

So yes, I don't see any place where the number of intermediaries increased

I meant - I am NOT worried - i.e. I think having a string here is perfectly fine since it's not on a hot path.

I don't have any problem with storing variant as a member, or using it as storage. I actually think variants are great.
I dislike the API that it brings in, though. I definitely understand your comment re/ having to implement an explicit move / copy operations.
I guess we could:

Keep as is

Provide a wrapper get-method that returns string_view which points to the right string

do my trick, but have the string inside a unique_ptr

always use a string (Roman's suggestion)
Your call :)

I see you decided to keep as is. I would still say it's not a great API to force a call to std::visit with a templated lambda instead of having a

string_view GetMessage() const { visit... }

To wrap that, but again, your call :)

For the case where you want to move if possible, you could have another version (GetMessageDestructive()?)

😝
As soon as it'll be used more than in once place I'll add those

chakaz · 2023-08-03T11:04:02Z

src/facade/reply_builder.cc

@@ -220,6 +220,14 @@ void RedisReplyBuilder::SendError(string_view str, string_view err_type) {
  }
 }

+void RedisReplyBuilder::SendError(ErrorReply error) {


I don't see that you move anything from error, so can you accept it by const&?

Not sure. Forgot to comment about it. I want the CapturingReplyBuilder to move out of the string so it can re-use its memory. The downside is that it requires passing the error by value everyewhere to comply with the interface

src/server/main_service.h

src/server/main_service.cc

romange · 2023-08-03T14:03:12Z

src/server/main_service.cc

    lock_guard lk(mu_);
    if (unknown_cmds_.size() < 1024)
      unknown_cmds_[cmd_str]++;
-    return false;
+
+    return facade::ErrorReply{StrCat("unknown command `", cmd_str, "`"), "unknown_cmd"};


can you add using facade::ErrorReply at the beginning?
too much horizontal space is wasted on these namespaces.

dranikpg · 2023-08-03T14:18:41Z

Fixed comments except those that are debatable 🙂

romange

LGTM at high level.
@chakaz will continue :)

chakaz · 2023-08-06T03:53:01Z

src/server/main_service.cc

  bool is_trans_cmd = CO::IsTransKind(cid->name());
-  bool under_script = bool(dfly_cntx->conn_state.script_info);
+  bool under_script = bool(dfly_cntx.conn_state.script_info);


while here, maybe instead of casting to bool do:

bool under_script = dfly_cntx.conn_state.script_info != nullptr;

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg · 2023-08-06T07:18:26Z

while here, maybe instead of casting to bool do:
bool under_script = dfly_cntx.conn_state.script_info != nullptr;

You already did it in your previous PR, rebased

Signed-off-by: Vladislav Oleshko <[email protected]>

chakaz · 2023-08-06T07:45:59Z

src/facade/facade_types.h

+  explicit ErrorReply(OpStatus status) : message{}, kind{}, status{status} {
+  }
+
+  std::variant<std::string, std::string_view> message;


I see you decided to keep as is. I would still say it's not a great API to force a call to std::visit with a templated lambda instead of having a

string_view GetMessage() const { visit... }

To wrap that, but again, your call :)

For the case where you want to move if possible, you could have another version (GetMessageDestructive()?)

dranikpg marked this pull request as ready for review August 3, 2023 10:25

dranikpg requested review from chakaz and romange and removed request for chakaz August 3, 2023 10:25

dranikpg commented Aug 3, 2023

View reviewed changes

dranikpg requested a review from chakaz August 3, 2023 10:27

chakaz reviewed Aug 3, 2023

View reviewed changes

romange reviewed Aug 3, 2023

View reviewed changes

dranikpg force-pushed the fix-command-verify branch from ff34107 to fe43785 Compare August 3, 2023 14:25

dranikpg requested review from chakaz and romange August 3, 2023 16:27

romange reviewed Aug 3, 2023

View reviewed changes

chakaz previously approved these changes Aug 6, 2023

View reviewed changes

dranikpg added 4 commits August 6, 2023 10:14

feat: Use const ConnectionContext in VerifyCommand

6e73d21

Signed-off-by: Vladislav Oleshko <[email protected]>

fix: move variant unwrapping

c6be83d

Signed-off-by: Vladislav Oleshko <[email protected]>

fix: fixes

f80f1b9

Signed-off-by: Vladislav Oleshko <[email protected]>

fix: fixes v2

c94998d

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg dismissed chakaz’s stale review via 62bfefc August 6, 2023 07:15

dranikpg force-pushed the fix-command-verify branch from fe43785 to 62bfefc Compare August 6, 2023 07:15

feat: Add SendError overload for capturing rb

f79e619

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the fix-command-verify branch from 62bfefc to f79e619 Compare August 6, 2023 07:20

dranikpg requested a review from chakaz August 6, 2023 07:41

chakaz approved these changes Aug 6, 2023

View reviewed changes

dranikpg merged commit 3bc1e26 into dragonflydb:main Aug 6, 2023

dranikpg deleted the fix-command-verify branch August 10, 2023 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use const ConnectionContext in VerifyCommand #1633

feat: Use const ConnectionContext in VerifyCommand #1633

dranikpg commented Aug 3, 2023 •

edited

Loading

dranikpg Aug 3, 2023

dranikpg Aug 3, 2023

dranikpg Aug 3, 2023

chakaz Aug 3, 2023

dranikpg Aug 3, 2023 •

edited

Loading

chakaz left a comment

chakaz Aug 3, 2023

chakaz Aug 3, 2023

dranikpg Aug 3, 2023

romange Aug 3, 2023

dranikpg Aug 3, 2023

dranikpg Aug 3, 2023

dranikpg Aug 3, 2023

romange Aug 3, 2023

chakaz Aug 6, 2023

chakaz Aug 6, 2023

dranikpg Aug 6, 2023

chakaz Aug 3, 2023

dranikpg Aug 3, 2023

romange Aug 3, 2023

dranikpg commented Aug 3, 2023

romange left a comment

chakaz Aug 6, 2023

dranikpg commented Aug 6, 2023

chakaz Aug 6, 2023

feat: Use const ConnectionContext in VerifyCommand #1633

feat: Use const ConnectionContext in VerifyCommand #1633

Conversation

dranikpg commented Aug 3, 2023 • edited Loading

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

dranikpg Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

chakaz left a 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

dranikpg commented Aug 3, 2023

romange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg commented Aug 6, 2023

Choose a reason for hiding this comment

dranikpg commented Aug 3, 2023 •

edited

Loading

dranikpg Aug 3, 2023 •

edited

Loading