Support MINID, NOMKSTREAM and LIMIT options for XADD command

[Abhra303]

Fixes #1163

XADD command currently doesn't support minid, nomkstream and limit options. This PR aims to support these options.

Additionally, I modified the pre-commit configuration file to correctly exclude multiple files (i.e. src/redis and ci/ directories).

[Abhra303]

Hey @romange @dranikpg @royjacobson , could you please rerun the tests and review the PR?

[dranikpg]

Great work! Happy to see we're getting support for more and more stream arguments. I've re-run the tests - all passed. Seemed like some internal issue within Github.

[dranikpg]

1. This can maybe be an enum inside AddOpts
2. Instead of using pure numbers we can include the TRIM_STRATEGY defines from redis (move to header before)

[Abhra303]

Yeah, Sure! I think point number 2 is better

[dranikpg]

Those are not exclusive 🙂

[Abhra303]

Could not we completely remove these const expressions and directly use the redis library's TRIM_STRATEGY definitions?

[Abhra303]

I.e. Something like if (add_opts.trim_strategy == TRIM_STRATEGY_MAXLEN && ...?

[dranikpg]

I think yes. enum TrimStrategy {MAX_LEN = TRIM_STRATEGY_MAXLEN, ... = ....} would be the cleanest, but that'll work too. Just that we don't have hard-coded integers

[Abhra303]

Ok, I am going with your suggestion then :)

[dranikpg]

Can you please make the limit test a little bit more sophisticated?

Another way to control the amount of work done by the command when using the ~, is the LIMIT clause. When used, it specifies the maximal count of entries that will be evicted

So what should limit 1 do here? It might have no effect because ~ is approximate

although after trimming, the stream may have few tens of additional entries over the threshold.

I'd suggest filling the stream with a few hundreds of entries and then setting a low target for maxlen, so that at least a few dozens will be evicted for sure - and test it. Then do the same, but now call the final xadd with limit and assert that only a few were removed

[Abhra303]

I was unsure about the test case of limit. So I used one of the redis's "limit" test case. I think your suggestion is great.

Adding these test cases! Thanks.

[dranikpg]

Great work! Happy to see we're getting support for more and more stream arguments

[Abhra303]

Oops, seems like I forgot to add the enum class you were suggesting about.

[royjacobson]

Hi @Abhra303!

I noticed that this PR causes some warnings when compiling:

[19/37] Building CXX object src/server/CMakeFiles/dragonfly_lib.dir/stream_family.cc.o
/home/roy/work/dragonfly/src/server/stream_family.cc: In function ‘facade::OpResult<streamID> dfly::{anonymous}::OpAdd(const dfly::OpArgs&, std::string_view, const dfly::{anonymous}::AddOpts&, facade::CmdArgList)’:
/home/roy/work/dragonfly/src/server/stream_family.cc:543:5: warning: missing initializer for member ‘streamAddTrimArgs::id’ [-Wmissing-field-initializers]
  543 |     };
      |     ^
/home/roy/work/dragonfly/src/server/stream_family.cc:543:5: warning: missing initializer for member ‘streamAddTrimArgs::id_given’ [-Wmissing-field-initializers]
/home/roy/work/dragonfly/src/server/stream_family.cc:543:5: warning: missing initializer for member ‘streamAddTrimArgs::seq_given’ [-Wmissing-field-initializers]
[...]

I'm pretty sure this is an actual bug and not a false positive in this case, since some of the uninitialized members are directly used in the follow up. Could you please take a look at the warnings and fix it?

Thanks!