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

onobc · 2025-12-02T03:32:48Z

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

See #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

Closes #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

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 · 2025-12-02T03:38:14Z

src/main/java/org/springframework/data/redis/connection/jedis/StreamConverters.java

 		if (options.hasTrimOptions()) {
 			TrimOptions trim = options.getTrimOptions();
-			var strategy = trim.getTrimStrategy();
+			TrimStrategy strategy = trim.getTrimStrategy();


I'm an advocate for var and I may bad-influenced you by using var in my previous commit or in some pseudo code when we were discussing things. If so, my bad. In general you will not find it in the main source of many Spring projects (sometimes in the tests).

Sure, let's keep using the proper type.

onobc · 2025-12-02T03:58:26Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+		private final TrimStrategy trimStrategy;
+		private final TrimOperator trimOperator;
+		private final @Nullable Long limit;
+		private final @Nullable StreamDeletionPolicy pendingReferences;


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.

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.

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.

Signed-off-by: Chris Bono <chris.bono@broadcom.com>

See #3232 Signed-off-by: Chris Bono <chris.bono@broadcom.com>

…options See #3232 Signed-off-by: Chris Bono <chris.bono@broadcom.com>

See #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

This commit makes the following "polish" changes: - remove manual not null check in `TrimOptions.<init>` as it's already `@NullMarked` - annnotate `RedisStreamCommands.XDelOptions` with `@NullMarked` - minor tweak to javadoc - remove usage of `var` in main code - use since `4.1` instead of `4.0` See #3232 Signed-off-by: Chris Bono <chris.bono@broadcom.com>

onobc · 2025-12-02T04:14:41Z

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 · 2025-12-02T10:05:41Z

src/test/java/org/springframework/data/redis/connection/jedis/JedisConvertersUnitTests.java

 			assertThat(params).extracting("expiration", "expirationValue").containsExactly(expirationType, expirationValue);
 		}
 	}
+


Nit: Formatting

mp911de · 2025-12-02T10:11:58Z

src/test/java/org/springframework/data/redis/connection/jedis/StreamConvertersUnitTest.java

+			RecordId recordId = RecordId.of("1234567890-0");
+			XAddOptions options = XAddOptions.none();
+
+			XAddParams params = StreamConverters.toXAddParams(recordId, options);


A testing approach of testing that a component is not doing something quickly leads to a lot of complexity as the subsequent lines unfold into several assertions. Instead, I suggest testing for the expectation that the component is doing something we expect, namely, not adding any command arguments.

XAddParams.addParams(CommandArguments) would be a neat way to utilize public API for testing which command arguments are added.

mp911de · 2025-12-02T10:14:02Z

src/test/java/org/springframework/data/redis/connection/jedis/StreamConvertersUnitTest.java

+	}
+
+	@Nested // GH-3232
+	class ToStreamDeletionPolicyShould {


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.

mp911de · 2025-12-02T10:18:31Z

src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionUnitTests.java

 		}

+		@Test // GH-3232
+		void xaddShouldHonorMinId() {


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.

mp911de · 2025-12-02T10:19:58Z

src/test/java/org/springframework/data/redis/core/DefaultStreamOperationsIntegrationTests.java

+		RecordId messageId = streamOps.add(key, Collections.singletonMap(hashKey, value));
+
+		streamOps.createGroup(key, ReadOffset.from("0-0"), "my-group");
+


Nit: Formatting

mp911de · 2025-12-02T10:33:16Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+		private final TrimStrategy trimStrategy;
+		private final TrimOperator trimOperator;
+		private final @Nullable Long limit;
+		private final @Nullable StreamDeletionPolicy pendingReferences;


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.

mp911de · 2025-12-02T10:34:30Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+		 * Prefer {@code XAddOptions.trim(TrimOptions.maxLen(n).approximate())} or
+		 * {@code XAddOptions.trim(TrimOptions.minId(id).exact())}.
 		 */
+		@Deprecated(since = "4.0", forRemoval = false)


I suggest avoiding forRemoval = true. It's a pretty harsh signal. Just the deprecation is a lighter form of signalling improper API usage, and that there is a better variant.

mp911de · 2025-12-02T10:34:58Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+		public XAddOptions nomkstream(boolean nomkstream) {
+			return new XAddOptions(nomkstream, trimOptions);
+		}
+


Nit: Formatting

mp911de · 2025-12-02T10:35:27Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+		 * The entry ID does not exist in the stream.
+		 */
+		NOT_FOUND(-1L),
+		/**


Nit: Formatting (missing blank line)

mp911de · 2025-12-02T10:35:40Z

src/main/java/org/springframework/data/redis/connection/RedisStreamCommands.java

+	 * @since 4.1
+	 */
+	enum StreamDeletionPolicy {
+		/**


Nit: Formatting (missing blank lines)

viktoriya-kutsarova added 2 commits December 1, 2025 21:31

Expand XADD and XTRIM with exact trimming, limit and deletion policy.

ba5b1c9

See #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

Add XACKDEL and XDELEX commands support.

32ae6db

Closes #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 2, 2025

onobc changed the title ~~WIP: Issue/3232~~ [upstream] Add support for XDELEX and XACKDEL, and expand options for XADD and XTRIM Dec 2, 2025

onobc commented Dec 2, 2025

View reviewed changes

onobc mentioned this pull request Dec 2, 2025

Add support for XDELEX and XACKDEL, and expand options for XADD and XTRIM #3247

Closed

9 tasks

onobc and others added 5 commits December 1, 2025 22:05

3232 - Prepare branch

168cf80

Signed-off-by: Chris Bono <chris.bono@broadcom.com>

Make XTrimOptions and XAddOptions share trim options

ee8bf90

See #3232 Signed-off-by: Chris Bono <chris.bono@broadcom.com>

Move trim options into top-level class that is used by XTrim and Xdd …

803d21d

…options See #3232 Signed-off-by: Chris Bono <chris.bono@broadcom.com>

Polishing stream commands changes

e1375ee

See #3232 Signed-off-by: viktoriya.kutsarova <viktoriya.kutsarova@redis.com>

onobc force-pushed the issue/3232 branch from c5ca410 to b4376d6 Compare December 2, 2025 04:05

mp911de requested changes Dec 2, 2025

View reviewed changes

		RecordId messageId = streamOps.add(key, Collections.singletonMap(hashKey, value));

		streamOps.createGroup(key, ReadOffset.from("0-0"), "my-group");

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

Are you sure you want to change the base?

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

Conversation

onobc commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onobc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onobc commented Dec 2, 2025

Uh oh!

mp911de left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

onobc commented Dec 2, 2025 •

edited

Loading

onobc left a comment •

edited

Loading