[upstream] Add support for XDELEX and XACKDEL, and expand options for XADD and XTRIM

[onobc]

This change introduces the Support XDELEX, XACKEX and extend XADD and XTRIM options feature...

See original pull request for more details.

Closes: #3232
Original Pull Request: #3247

[onobc]

Note

Thank you @viktoriya-kutsarova for the excellent deprecated detailed descriptions tells users what replacement to use w/o them having to do software archeology to figure it out. :)

Once Jenkins and PR is green and we resolve the rename concern (#3272 (comment)) this will be ready to merge.

Also note that I will do some final massaging of the commits (couple of squashes and be sure they proper links are in the commits etc..).

Thanks

[onobc]

I do have a small concern about possible confusion caused from tbedeletionPolicy/getPendingReferences rename. I believe now that the benefit of a better high-level api name pendingRefereneces does not outweigh the inconsistency between the two names throughout. I am leaning towards consistency here and just leave it named deletionPolicy for now. I would like to get yours MP input on this topic.

[mp911de]

I suspect that I came up with the name of pendingReferences and StreamDeletionPolicy (not really sure about whether it was me or someone else). In any case, both terms describe concepts. Redis command documentation says:

Specifies how to handle consumer group references when trimming. If there are no consumer groups, these arguments have no effect. Available since Redis 8.2.
When trimming, removes entries … preserves existing references to these entries in all consumer groups' PEL (Pending Entries List).

Redis sometimes isn't great at making some concepts explicit, instead the underlying concept leaks through documentation. I think StreamDeletionPolicy is the shortest possible name to express a deletion policy for stream entry references from the pending element list. Agreed that the combination is not super-self explanatory, however it is something that when I encounter, I would want to read up more what this is. PendingElementListDeletionPolicy could be an alternative, yet its wordiness is much more annoying than the state we are currently at.

[viktoriya-kutsarova]

It looks like I introduced some inconsistency with the rename, so I want to make sure I understand: are we discussing the naming of the class (StreamDeletionPolicy), the field (currently pendingReferences), or both?

Personally, I would stick with streamDeletionPolicy for both. Based on the Redis documentation, all three options (KEEPREF | DELREF | ACKED) represent different policies for how entries are removed from the stream. KEEPREF and DELREF delete entries strictly according to the trimming strategy (differing only in how PEL references are handled), while ACKED deletes only entries that were read and acknowledged by all consumer groups. In that sense, streamDeletionPolicy feels like the most concise name that still reflects the underlying concept.

As you noted, consistency across the API is important, so aligning on a single name and using it consistently seems like the best approach.

[onobc]

cc: @viktoriya-kutsarova @mp911de

[mp911de]

I've left a few comments. There are several public API elements (methods, classes), that are missing Javadoc along with a mix of im proper or missing @since tags. I strongly urge to refrain from Deprecated(forRemoval=true) as the current API isn't breaking anything nor imposes its usage any danger, we've just broadened feature support.

Test code is overly verbose in its current form as each test scenratio is unfolded in a separate method rather than utilizing parametrized tests. In some areas we follow a test approach that doesn't consider refactoring of the underlying code inside components that we're testing. These require cleanup along with proper formatting according to our code style.

[mp911de]

Instead of having each invariant represented by a test method, can we turn this into parametrized tests if you want to cover each code path in tests? The current arrangement is pretty verbose requiring a lot of mental energy to parse those tests. Using a parametrized test condenses the setup into an assertion and a set of input/expected arguments.

[onobc]

I reworked the test to use @ParameterizedTest throughout. It definitely makes it more concise. In order to keep the context of each test I added a name to it. The tests are also named and arranged intentionally to produce a readable result in the test report:

[mp911de]

Testing individual arguments in the xadd command series deviates from the underlying code.

Clearly, previously, the conversion has happened in the xadd method itself. With this pull request, all command conversion has been refactored to a converter, so it would be sufficient to keep a single xadd test with options and remove all other test variants to align with the code under test.

[onobc]

so it would be sufficient to keep a single xadd test with options and remove all other test variants to align with the code under test.

There is no StreamConverterUnitTests for Lettuce like there is for Jedis. So we would need to create a Lettuce StreamConverterUnitTests to cover the various test cases.

It is also worth pointing out that many of the other APIs under test also delegate to converters yet the tests are implemented in here (same case as xtrim / xadd). Those could also be "moved" into the StreamConverterUnitTests and hence more tests simplified.

I do think this would be a good thing to do but I am hesitant to do it just for xadd / xtrim (focus of GH-3232) because we would end up w/ 3/4 of the tests being done one way and the other 1/4 another way.
On the other hand, the concern of waiting to "do it all in a other PR" is that PR may never come to fruition.

Would you prefer to do this all at once or incrementally, starting w/ xtrim and xadd?

[mp911de]

I do think this would be a good thing to do but I am hesitant to do it just for xadd / xtrim (focus of #3232) because we would end up w/ 3/4 of the tests being done one way and the other 1/4 another way.
On the other hand, the concern of waiting to "do it all in a other PR" is that PR may never come to fruition.

I think we should wrap up this effort at some point and so stretching polishing beyond a sane point is rather preventing a wrap-up. I suggest creating a follow-up ticket to migrate those tests. What do you think?

[mp911de]

I suspect that I came up with the name of pendingReferences and StreamDeletionPolicy (not really sure about whether it was me or someone else). In any case, both terms describe concepts. Redis command documentation says:

Specifies how to handle consumer group references when trimming. If there are no consumer groups, these arguments have no effect. Available since Redis 8.2.
When trimming, removes entries … preserves existing references to these entries in all consumer groups' PEL (Pending Entries List).

Redis sometimes isn't great at making some concepts explicit, instead the underlying concept leaks through documentation. I think StreamDeletionPolicy is the shortest possible name to express a deletion policy for stream entry references from the pending element list. Agreed that the combination is not super-self explanatory, however it is something that when I encounter, I would want to read up more what this is. PendingElementListDeletionPolicy could be an alternative, yet its wordiness is much more annoying than the state we are currently at.