chore(reply_builder): add dcheck that each command invocation has rep… #2067

kostasrim · 2023-10-24T17:45:51Z

Some of the replication tests fail, and I suspect it's our little bug :) I am investigating and will update

…lied

kostasrim · 2023-10-24T18:00:22Z

src/facade/reply_builder.h

@@ -134,6 +137,8 @@ class SinkReplyBuilder {

  // Similarly to batch mode but is controlled by at operation level.
  bool should_aggregate_ : 1;
+  bool has_replied_ : 1;
+  bool has_batch_replied_ : 1;


accidental will remove

romange · 2023-10-24T18:31:17Z

Some of the tests failing are around memcache flow that have a dedicated reply builder

dranikpg · 2023-10-24T20:40:52Z

src/server/main_service.cc


+  cntx->reply_builder()->ExpectReply();
  try {
    cid->Invoke(tail_args, cntx);
  } catch (std::exception& e) {
    LOG(ERROR) << "Internal error, system probably unstable " << e.what();
    return false;
  }
+  DCHECK(cntx->reply_builder()->HasReplied());



would be my solution suggestion, not sure if its simpler 🙂

#ifdef NDEBUG CapturingReplyBuilder crb{ReplyMode::ALL}; auto* old = cntx->Inject(&crb); #endif *invoke* #ifdef NDEBUG auto reply = crb.Take(); DCHECK(reply.index() > 0); CRB::Apply(std::move(reply), (*cntx).operator->()); cntx->Inject(old); #endif

I have some of the mechanics as a convenient RAII in 1924, you could steal it from there if you choose this approach, so all the code doesn't pollute this small function

It would not work with memcached, because the Apply function accepts a RedisReplyBuilder. Moreover, for some reason it made some of the tests stack.

However, I really like this idea, IMO, it's cleaner. What I will try on a seprate PR (because I don't want to block us) is:

Overload Apply to work with MCBuilder (or just make it accept a SinkReplyBuilder).

Figure out what cause the hangs in the tests.

I'd suggest just doing this for non memcached commands because CRB has no support for storing mc-like replies

Other way round its likely not worth the effort

I'd suggest just doing this for non memcached commands because CRB has no support for storing mc-like replies

So just skip for the memcached commands? I can do this by dyn_cast and checking if the reply builder is MCBuilder. I will try this out

But second thoughts we would also miss the dcheck for memcached commands which I am not really happy about... @romange any opinion on this?

@dranikpg I applied your comments, I don't want to delay this any longer. Took a little bit more time, because I had to add a few exceptions but it passes locally

I do not care about missing memcache - it has a small surface and it does not change a lot.

i saw now the previous commits. I prefer the boolean option

dranikpg

Just merge it, let's not spend this much time discussing such a small issue I agree

dranikpg · 2023-10-25T16:12:22Z

src/server/main_service.cc

+  const bool is_repl_conf = cid->name() == "REPLCONF";                                      \
+  const bool is_dfly = cid->name() == "DFLY";                                               \
+  const bool is_exec = cid->name() == "EXEC";                                               \


bool ignore = set{"REPLCONF", "DFLY", "EXEC"}.count(cid->name()); 😭

dranikpg · 2023-10-25T16:16:21Z

src/server/main_service.cc

+  absl::Cleanup clean = [cntx, old]() {                                                     \
+    if (old)                                                                                \
+      cntx->Inject(old);                                                                    \
+  };                                                                                        \


what I initially thought was this

struct ReplyGuard {} // do all the stuff in contructor and destructor Invoke() { #ifdef NDEBUG ReplyGuard guard; #endif ... }

romange · 2023-10-25T16:29:21Z

src/server/main_service.cc

@@ -1047,6 +1047,39 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
  }
 }

+#ifndef NDEBUG
+
+#define ENABLE_DCHECK_REPLIED                                                               \


is it the only way to do it? by injecting a reply builder and bunch of preprocesor code?

Injecting the CapturingReplyBuilder is clean enough and I cleaned up the preprocessor code. Do you have any better suggestions?

romange · 2023-10-25T17:07:09Z

src/server/main_service.cc

+    if (old) {
+      auto reply = crb.Take();
+      DCHECK(reply.index() > 0);
+      CapturingReplyBuilder::Apply(std::move(reply), old);


much nicer! My question is - could it be that injecting CapturingReplyBuilder would actually change the debug version significantly from the release in terms of I/O pattern?

somewhat, now replies are sent only once the command handler finished executing

It does delay the replies after the invocation of the command. So yes, it does alter the behavior. Originally, I had it implemented differently (see the early commits) but the CapturingReplyBuilder seems a clearer approach.

would actually change the debug version significantly from the release in terms of I/O pattern?

I think it also changes the functionality, as I described above which I agree it is not great but in this context maybe it's ok if you take into account that the release version is also different because of different optimizations.

There is another solution in the previous commits (which can be cleaned up as well) but this also pollutes the RedisReplyBuilder (but it doesn't delay the replies). What's your take?

I have not see your solution but is not it just about setting a boolean in a few low-level functions?

i mean in replybuilder ? I am ok with this solution but I do preference with poluting replybuilder if it is simple and efficient.

romange · 2023-10-26T09:44:22Z

src/facade/reply_builder.cc

@@ -98,6 +99,14 @@ void SinkReplyBuilder::SendRaw(std::string_view raw) {
  Send(&v, 1);
 }

+void SinkReplyBuilder::ExpectReply() {
+  has_replied_ = true;


What does it mean?

it should be false hold

romange · 2023-10-26T09:45:10Z

src/server/main_service.cc

+            .contains(cid_name);
+    const bool should_dcheck = !is_one_of && !is_script;
+    if (should_dcheck) {
+      builder->ExpectReply();


I do not understand this logic.

It should have been false. Basically, it sets that we expect the reply on each Invoke which internally sets the has_replied_ to false. If the reply builder is used to reply, then this is set to true, which we DCHECK on destructor

chore(reply_builder): add dcheck that each command invocation has rep…

ff10ea7

…lied

kostasrim force-pushed the add_dcheck_for_replies branch from 222bfee to ff10ea7 Compare October 24, 2023 17:47

kostasrim commented Oct 24, 2023

View reviewed changes

kostasrim changed the title ~~chore(reply_builder): add dcheck that each command invocation has rep…~~ (WIP) chore(reply_builder): add dcheck that each command invocation has rep… Oct 24, 2023

dranikpg reviewed Oct 24, 2023

View reviewed changes

kostasrim added 2 commits October 25, 2023 12:50

fix tests

1f575bb

fix memcached

57783e6

kostasrim changed the title ~~(WIP) chore(reply_builder): add dcheck that each command invocation has rep…~~ chore(reply_builder): add dcheck that each command invocation has rep… Oct 25, 2023

kostasrim added 2 commits October 25, 2023 13:41

remove unused log

bdf486c

apply gh suggestions

cab0625

kostasrim self-assigned this Oct 25, 2023

kostasrim requested a review from dranikpg October 25, 2023 16:01

dranikpg previously approved these changes Oct 25, 2023

View reviewed changes

romange reviewed Oct 25, 2023

View reviewed changes

cleanup

719ec59

kostasrim dismissed dranikpg’s stale review via 719ec59 October 25, 2023 17:01

romange reviewed Oct 25, 2023

View reviewed changes

revert back to boolean check

a09f476

kostasrim requested review from romange and dranikpg October 26, 2023 07:52

romange reviewed Oct 26, 2023

View reviewed changes

bug fixes

45d1839

kostasrim requested a review from romange October 26, 2023 10:10

romange approved these changes Oct 26, 2023

View reviewed changes

romange merged commit b655fc7 into main Oct 26, 2023
10 checks passed

romange deleted the add_dcheck_for_replies branch October 26, 2023 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(reply_builder): add dcheck that each command invocation has rep… #2067

chore(reply_builder): add dcheck that each command invocation has rep… #2067

kostasrim commented Oct 24, 2023

kostasrim Oct 24, 2023

romange commented Oct 24, 2023

dranikpg Oct 24, 2023 •

edited

Loading

dranikpg Oct 24, 2023 •

edited

Loading

kostasrim Oct 25, 2023

dranikpg Oct 25, 2023

dranikpg Oct 25, 2023

kostasrim Oct 25, 2023

kostasrim Oct 25, 2023

kostasrim Oct 25, 2023

romange Oct 25, 2023

romange Oct 25, 2023 •

edited

Loading

dranikpg left a comment

dranikpg Oct 25, 2023

dranikpg Oct 25, 2023

romange Oct 25, 2023

kostasrim Oct 25, 2023 •

edited

Loading

romange Oct 25, 2023

dranikpg Oct 25, 2023 •

edited

Loading

kostasrim Oct 25, 2023 •

edited

Loading

romange Oct 25, 2023

romange Oct 25, 2023

romange Oct 26, 2023

kostasrim Oct 26, 2023

romange Oct 26, 2023

kostasrim Oct 26, 2023

chore(reply_builder): add dcheck that each command invocation has rep… #2067

chore(reply_builder): add dcheck that each command invocation has rep… #2067

Conversation

kostasrim commented Oct 24, 2023

Choose a reason for hiding this comment

romange commented Oct 24, 2023

dranikpg Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

dranikpg Oct 24, 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

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 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

dranikpg 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

kostasrim Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

kostasrim Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg Oct 24, 2023 •

edited

Loading

dranikpg Oct 24, 2023 •

edited

Loading

romange Oct 25, 2023 •

edited

Loading

kostasrim Oct 25, 2023 •

edited

Loading

dranikpg Oct 25, 2023 •

edited

Loading

kostasrim Oct 25, 2023 •

edited

Loading