feat(streams): support XACK command #1923

theyueli · 2023-09-24T09:43:15Z

src/server/stream_family.cc

chakaz · 2023-09-26T06:04:16Z

src/server/stream_family.cc

+
+  OpResult<uint32_t> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
+  if (result || result.status() == OpStatus::KEY_NOTFOUND) {
+    return (*cntx)->SendLong(*result);


Wouldn't *result crash if result.status() == OpStatus::KEY_NOTFOUND?
Perhaps you could add an if before, something along the lines of:

if (result.status() == OpStatus::KEY_NOTFOUND) { result = 0UL; } if (result) { // send long... }

sorry.. I just verified again. when key is not found, the *result is actually 0...

Yes +1, same for when the cgroup does not exist. Good catch @theyueli

chakaz · 2023-09-26T06:06:31Z

Oh, and I just noticed there are no tests (which could have caught the NOTFOUND issue), could you add some please? 🙏

src/server/stream_family.cc

theyueli · 2023-09-26T09:05:34Z

Oh, and I just noticed there are no tests (which could have caught the NOTFOUND issue), could you add some please? 🙏

Yes I was working on that, now they are complete.

kostasrim

Good work 👨‍🍳 @theyueli`

Very small nit to replace NULL with nullptr and I will approve.

src/server/stream_family.cc

kostasrim · 2023-09-26T17:03:55Z

src/server/stream_family.cc

+  string_view key = ArgS(args, 0);
+  string_view group = ArgS(args, 1);
+  args.remove_prefix(2);
+  absl::InlinedVector<streamID, 8> ids(args.size());


kostasrim · 2023-09-26T17:07:03Z

src/server/stream_family.cc

@@ -1444,7 +1476,7 @@ void DestroyGroup(string_view key, string_view gname, ConnectionContext* cntx) {
    return OpDestroyGroup(t->GetOpArgs(shard), key, gname);
  };

-  OpStatus result = cntx->transaction->ScheduleSingleHop(std::move(cb));
+  OpStatus result = cntx->transaction->ScheduleSingleHop(cb);


💯 for making the changes across the file

question, what's the reason for removing move?

I think that the claim is that it doesn't do anything (because all captures are by ref, and so "moving" pointers is the same as copying them).
I would, however, counter-claim that it's a better practice to use move even in such cases, because there's no harm, and it's future proof and does not require carefully reviewing the capture list..

I have not seen the original request but +1 to what @chakaz said. If tomorrow cb will capture a 1MB string, it will be copied, and performance bugs are hard to catch.

ok, I'm good with either way. I will reverse these changes.

Small rant vol2, it captures everything by reference, even if we added a string, we would need to explicitly copy it by value making it nearly impossible for this to happen unless you really want to copy this 1mb string...

Re/ 1: I'd rather have longer compilation times than runtime. When a stressed compiler will be an issue, we can tackle that (and I doubt that this will be the cause of pain for the compiler). I'd even bet that <utility> is included in every (or 99%) of TUs we use (either directly or indirectly).
Re/ 2: I'd argue that:

It's way more common to capture more local variables (we all add them, all the time) than to add members to structs, and so it makes sense to have a different rule of thumb

If you add a 1mb string to your class, it might make sense to also add a move-constructor (if one is not generated for you)

It's much less noisier to move callbacks than to move all structs that are ever assigned :)

Small rant vol2, it captures everything by reference, even if we added a string, we would need to explicitly copy it by value making it nearly impossible for this to happen unless you really want to copy this 1mb string...

Simple grepping found 2 cases in which we capture by-ref by-default, but also some vars by value:
https://github.com/romange/helio/blob/937652d5803596893aa774206b36d7b91dccf883/util/fibers/fiberqueue_threadpool.h#L108
https://github.com/romange/helio/blob/937652d5803596893aa774206b36d7b91dccf883/util/fibers/fiberqueue_threadpool.h#L122

If you add a 1mb string to your class, it might make sense to also add a move-constructor (if one is not generated for you)

yes but to trigger it you need to call std::move everywhere -- ouch!

Simple grepping found 2 cases in which we capture by-ref by-default, but also some vars by value:

Both of them capture a Done object which internally holds an intrusive_ptr. So no deep copying whatsoever...
And if you tell me that if somebody adds a 1mb string to the class Done I would say it would have the same effect as:

somewhere in our codebase:

void some_func_call(Done done) { } void foo() { Done copy_1mb; some_func_call(copy_1mb); }

Once you add that 1mb string, it will have the same effect for any function that accepts a Done object

Sorry Shahar, I really don't see any value in using std::move() like that. It's noise, even for (1) above, if you add a non trivial type either move it or capture it by reference (the same you would do for any object IMO). Again, since you and Roman agree, I think we got a consensus. I don't want to push staff neither do I think this move is the end of the world or high in importance.

kostasrim · 2023-09-26T17:21:34Z

src/server/stream_family.cc

+  auto [stream_inst, cg] = *res;
+
+  if (cg == nullptr || stream_inst == nullptr) {
+    return 0;


Originally, I wrote this long message here cause I thought you were transforming an error to a valid result. Upon testing this on redis (and going over the ACK docs) it seems to be correct. Therefore, both if the cg and the key does not exist we should return 0.

kostasrim · 2023-09-26T17:29:09Z

src/server/stream_family.cc

+
+  OpResult<uint32_t> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
+  if (result || result.status() == OpStatus::KEY_NOTFOUND) {
+    return (*cntx)->SendLong(*result);


Yes +1, same for when the cgroup does not exist. Good catch @theyueli

theyueli · 2023-09-26T22:12:52Z

Good work 👨‍🍳 @theyueli`

Very small nit to replace NULL with nullptr and I will approve.

Thanks! fixed.

feat(streams): support XACK command

8396e8e

theyueli added the enhancement New feature or request label Sep 24, 2023

theyueli marked this pull request as draft September 24, 2023 09:43

chakaz reviewed Sep 26, 2023

View reviewed changes

functional tests

6fbf524

theyueli marked this pull request as ready for review September 26, 2023 08:43

review comments

e69fb2a

kostasrim reviewed Sep 26, 2023

View reviewed changes

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

theyueli added 2 commits September 26, 2023 02:06

review comments

289a0d4

review comments

b021e3c

kostasrim reviewed Sep 26, 2023

View reviewed changes

theyueli added 2 commits September 26, 2023 10:39

review comments

9132572

review comments

a9a8ff0

kostasrim approved these changes Sep 27, 2023

View reviewed changes

theyueli merged commit c47469e into dragonflydb:main Sep 27, 2023
7 checks passed

FZambia mentioned this pull request Sep 27, 2023

XREADGROUP issues: DF consumes from non-existing stream and does not block on newly created stream #1860

Closed

theyueli self-assigned this Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(streams): support XACK command #1923

feat(streams): support XACK command #1923

theyueli commented Sep 24, 2023

chakaz Sep 26, 2023

theyueli Sep 26, 2023

kostasrim Sep 26, 2023

chakaz commented Sep 26, 2023

theyueli commented Sep 26, 2023

kostasrim left a comment

kostasrim Sep 26, 2023

kostasrim Sep 26, 2023

romange Sep 26, 2023 •

edited

chakaz Sep 26, 2023

romange Sep 26, 2023

theyueli Sep 26, 2023

kostasrim Sep 27, 2023 •

edited

chakaz Sep 27, 2023

chakaz Sep 27, 2023

kostasrim Sep 27, 2023 •

edited

kostasrim Sep 27, 2023

kostasrim Sep 26, 2023

kostasrim Sep 26, 2023

theyueli commented Sep 26, 2023

feat(streams): support XACK command #1923

feat(streams): support XACK command #1923

Conversation

theyueli commented Sep 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented Sep 26, 2023

theyueli commented Sep 26, 2023

kostasrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Sep 26, 2023 • edited

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 Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 27, 2023 • edited

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

theyueli commented Sep 26, 2023

romange Sep 26, 2023 •

edited

kostasrim Sep 27, 2023 •

edited

kostasrim Sep 27, 2023 •

edited