Search - request/response policies implementation with API overrides #3465

a-TODO-rov · 2025-10-05T13:19:05Z

Implementing request/response policies for search - version with API overrides
Version with ClusterWriter routing implementation - #3409
Part of #3447

…sor.

Change sticky cursor implementation to propagate more lightweight Cursor object, instead the whole AggregationReply.

Get connection by nodeId

Add FT.ALTER, FT.DROPINDEX (and DD), FT.SYNDUMP, FT.SYNUPDATE

Copilot

Pull Request Overview

This PR implements request/response policies for RediSearch in cluster environments, transitioning from cursor ID-based routing to cursor object-based routing with API overrides. The implementation enables keyless RediSearch commands to route randomly across cluster nodes while respecting ReadFrom policies and maintaining cursor stickiness.

Key changes:

Replaced cursor ID parameters with Cursor objects containing both cursor ID and node ID for cluster routing
Added cluster-aware routing for keyless RediSearch commands that honor ReadFrom policies
Updated cursor lifecycle to support sticky routing in cluster mode

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`AggregationReply.java`	Replaced cursor ID field with optional `Cursor` class containing cursor ID and node ID
`AbstractRedisAsyncCommands.java`	Updated cursor method signatures to accept `Cursor` objects instead of cursor IDs
`RedisAdvancedClusterAsyncCommandsImpl.java`	Added cluster routing implementation for keyless RediSearch commands
`PooledClusterConnectionProvider.java`	Added random connection selection method for keyless command routing
Template and interface files	Updated API signatures across sync/async/reactive interfaces to use `Cursor` objects
Test files	Updated integration tests to use new cursor API and verify cluster routing behavior

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-09T09:35:35Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java

+import io.lettuce.core.search.arguments.SynUpdateArgs;
 import reactor.core.publisher.Mono;

+import java.util.Optional;


The import is placed after other package imports. Move it to the appropriate alphabetical position with other java.util imports around line 83.

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

Copilot · 2025-10-09T09:35:35Z

src/test/java/io/lettuce/core/search/RediSearchAggregateIntegrationTests.java

+        // RediSearch may either omit the cursor on the final page, or return a non-zero cursor
+        // that requires one more empty READ to return 0. Be tolerant across versions.
+        long effective = nextResult.getCursor().map(AggregationReply.Cursor::getCursorId).orElse(0L);


[nitpick] Consider moving this comment closer to the conditional logic on line 1354 where it's actually used, or make the comment more specific about which versions exhibit this behavior.

Suggested change

// RediSearch may either omit the cursor on the final page, or return a non-zero cursor

// that requires one more empty READ to return 0. Be tolerant across versions.

long effective = nextResult.getCursor().map(AggregationReply.Cursor::getCursorId).orElse(0L);

long effective = nextResult.getCursor().map(AggregationReply.Cursor::getCursorId).orElse(0L);

// RediSearch (prior to v2.6) may either omit the cursor on the final page, or return a non-zero cursor

// that requires one more empty READ to return 0. Be tolerant across versions.

Copilot · 2025-10-09T09:35:36Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java

+    private ConnectionIntent getConnectionIntent(ProtocolKeyword commandType) {
+        try {
+            RedisCommand probe = new Command(commandType, null);
+            boolean isReadOnly = getStatefulConnection().getOptions().getReadOnlyCommands().isReadOnly(probe);
+            return isReadOnly ? ConnectionIntent.READ : ConnectionIntent.WRITE;
+        } catch (Exception e) {
+            logger.error("Error while determining connection intent for " + commandType, e);
+            return ConnectionIntent.WRITE;
+        }
+    }


This method is duplicated in both RedisAdvancedClusterReactiveCommandsImpl and RedisAdvancedClusterAsyncCommandsImpl. Consider extracting this to a shared utility class or abstract base class to avoid code duplication.

tishun

Nits only except the connection intent

tishun · 2025-10-09T11:04:00Z

src/main/java/io/lettuce/core/cluster/AsyncClusterConnectionProvider.java

+     * Obtain a node-scoped connection for the given {@link ConnectionIntent} by first selecting a random upstream (master)
+     * shard to ensure even distribution across partitions and then delegating to the slot-based selection logic.


Typically, especially in an interface, you would not explain the implementation, but rather focus on the goal : in this case we do not care HOW we obtain a node, what we care is that we are obtaining a random node from the list of all nodes in the topology, by also considering any readfrom policies.

This is because the actual implementation might change, but the overall method contract / purpose could remain the same.

tishun · 2025-10-09T11:09:43Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

+            return new PipelinedRedisFuture<>(failed);
+        }
+        String nodeId = nodeIdOpt.get();
+        StatefulRedisConnection<K, V> byNode = getStatefulConnection().getConnection(nodeId, ConnectionIntent.WRITE);


Is it possible that a cursor is created on a READ node? Would this logic work in this case?

E.g. FT.AGGREGATE -> route to random node that is READ -> connection intent is WRITE here.

Well it seems in this case intent doesn't really matter much. We are acquiring connection to a specific redis-server instance by nodeId. For better semantics i will change intent READ for ftCursorread and leave intent WRITE for ftCursordel

tishun · 2025-10-09T11:22:45Z

src/main/java/io/lettuce/core/cluster/PooledClusterConnectionProvider.java

    }

+    @Override
+    public CompletableFuture<StatefulRedisConnection<K, V>> getRandomConnectionAsync(ConnectionIntent connectionIntent) {


Can we add a unit test for this method

tishun · 2025-10-09T11:23:07Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

+    @Override
+    public RedisFuture<AggregationReply<K, V>> ftCursorread(String index, Cursor cursor, int count) {


Can we add a unit test for this method (and reactive counterpart)

tishun · 2025-10-09T11:23:28Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

+    }
+
+    @Override
+    public RedisFuture<String> ftCursordel(String index, Cursor cursor) {


Can we add a unit test for this method (and reactive counterpart)

tishun · 2025-10-09T11:23:32Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

+     * @param <R> result type
+     * @return RedisFuture wrapping the routed execution
+     */
+    <R> RedisFuture<R> routeKeyless(Supplier<RedisFuture<R>> superCall,


Can we add a unit test for this method (and reactive counterpart)

tishun · 2025-10-09T11:23:36Z

src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java

+     * @param <R> result type
+     * @return RedisFuture wrapping the routed execution
+     */
+    <R> RedisFuture<R> routeKeyless(Supplier<RedisFuture<R>> superCall,


Can we add a unit test for this method (and reactive counterpart)

a-TODO-rov added 24 commits October 5, 2025 15:02

CAE-534 - Add sticky node connection support for ftAggregate with cur…

2ca3c59

…sor.

Implement random LB for RediSearch read commands.

b12e9c4

Change sticky cursor implementation to propagate more lightweight Cursor object, instead the whole AggregationReply.

Sync templates and API generator

2efe9ca

Add ftCreate.

e28b542

Get connection by nodeId

Improvements

50e5a64

equalize reactive

a7b0aec

Format

d97f3a0

Fix failing test

224eef2

Format

56df0fd

Code cleanup.

e714e25

Add FT.ALTER, FT.DROPINDEX (and DD), FT.SYNDUMP, FT.SYNUPDATE

Revert unintentional changes

4f9c2f2

Return override to dbSize

963598f

Account ReadFrom user preferences

e1e69ec

Minor fixes from code review

1d10889

Add readOnly RediSearch commands to default readOnly commands

88f5c58

Fix test

fe2b905

Fix test on lower env

45c074d

Keyless API

270dc64

Remove keys from advanced commands

632faf8

Minor fixes

4bc063e

Fix by templates

1f7a97f

Add test

ba968b5

Rename method

7bbb6e6

Fix tests

e5a82ba

a-TODO-rov requested review from Copilot, tishun and uglide October 9, 2025 09:33

Copilot AI reviewed Oct 9, 2025

View reviewed changes

tishun mentioned this pull request Oct 9, 2025

RediSearch - keyless routing & stickiness #3409

Closed

tishun approved these changes Oct 9, 2025

View reviewed changes

		* Obtain a node-scoped connection for the given {@link ConnectionIntent} by first selecting a random upstream (master)
		* shard to ensure even distribution across partitions and then delegating to the slot-based selection logic.

		@Override
		public RedisFuture<AggregationReply<K, V>> ftCursorread(String index, Cursor cursor, int count) {

Search - request/response policies implementation with API overrides #3465

Are you sure you want to change the base?

Search - request/response policies implementation with API overrides #3465

Uh oh!

Conversation

a-TODO-rov commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

tishun 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

a-TODO-rov commented Oct 5, 2025 •

edited

Loading