-
Notifications
You must be signed in to change notification settings - Fork 0
Source main #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Source main #2
Conversation
Nowadays, AI reviewing tools like CoPilot catch spellcheck issues better than pyspelling without a burden of maintaining wordlist.txt
* Add getCodec method to StatefulRedisClusterConnectionImpl * feat: Added `StatefulConnection.getCodec()` method * Formatting fix and and some re-wording * One file was missed in the original change --------- Co-authored-by: Tihomir Mateev <[email protected]>
…edis#3465) * CAE-534 - Add sticky node connection support for ftAggregate with cursor. * Implement random LB for RediSearch read commands. Change sticky cursor implementation to propagate more lightweight Cursor object, instead the whole AggregationReply. * Sync templates and API generator * Add ftCreate. Get connection by nodeId * Improvements * equalize reactive * Format * Fix failing test * Format * Code cleanup. Add FT.ALTER, FT.DROPINDEX (and DD), FT.SYNDUMP, FT.SYNUPDATE * Revert unintentional changes * Return override to dbSize * Account ReadFrom user preferences * Minor fixes from code review * Add readOnly RediSearch commands to default readOnly commands * Fix test * Fix test on lower env * Keyless API * Remove keys from advanced commands * Minor fixes * Fix by templates * Add test * Rename method * Fix tests * Polishing after code review
概述从primitives 变更
序列图sequenceDiagram
actor Client
participant ClusterConn as Redis Cluster<br/>Connection
participant Router as Keyless<br/>Router
participant Node as Random<br/>Node
Note over Client: FT.AGGREGATE (无键)
Client->>ClusterConn: ftAggregate(index, query)
ClusterConn->>Router: routeKeyless(superCall,<br/>routedCall, FT_AGGREGATE)
Router->>ClusterConn: getRandomStatefulConnection(READ)
ClusterConn->>Node: get connection to node
Node-->>Router: StatefulRedisConnection
Router->>Node: command via routing connection
Node-->>Router: AggregationReply with cursor
Router->>Router: stamp nodeId into cursor
Router-->>ClusterConn: AggregationReply<nodeId>
ClusterConn-->>Client: reply with nodeId
Note over Client: FT.CURSOR READ (使用nodeId)
Client->>ClusterConn: ftCursorread(index, cursor)
ClusterConn->>Router: extract nodeId from cursor
Router->>Node: route to specific node
Node->>Node: ftCursorread(index, cursorId)
Node-->>Router: AggregationReply
Router-->>ClusterConn: result
ClusterConn-->>Client: cursor results
预估代码审查工作量🎯 4 (复杂) | ⏱️ ~60 分钟 复杂性因素:
可能相关的PRs
诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.6)src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.javasrc/main/java/io/lettuce/core/AbstractRedisReactiveCommands.javasrc/test/java/io/lettuce/core/search/RediSearchAggregateIntegrationTests.javaComment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/templates/io/lettuce/core/api/RediSearchCommands.java (1)
9-26: 缺少 Cursor 导入导致编译失败模板中直接使用
Cursor,但未导入io.lettuce.core.search.AggregationReply.Cursor,生成的 Java 代码将出现无法解析符号的编译错误。请补充导入。import io.lettuce.core.search.AggregationReply; +import io.lettuce.core.search.AggregationReply.Cursor; import io.lettuce.core.search.SearchReply;src/main/java/io/lettuce/core/api/async/RediSearchAsyncCommands.java (1)
1173-1174: 文档不一致:应为 AggregationReply 而非 SearchReply游标读尽时的说明应对应本方法返回的
AggregationReply,而不是SearchReply。建议:
- * 当游标耗尽…返回的 {@link SearchReply} 将具有 cursor id 为 0。 + * 当游标耗尽(无更多结果)时,返回的 {@link AggregationReply} 将携带 cursor id 为 0。src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (1)
1173-1184: KDoc 返回类型描述错误:应为 AggregationReply游标耗尽时的说明应与方法返回类型一致:
- * 当游标耗尽(无更多结果)时,返回的 [SearchReply] 将具有 cursor id 为 0。 + * 当游标耗尽(无更多结果)时,返回的 [AggregationReply] 将具有 cursor id 为 0。src/main/java/io/lettuce/core/cluster/api/sync/NodeSelectionSearchCommands.java (1)
1166-1176: Javadoc 不一致:返回类型是 AggregationReply,而非 SearchReply该段描述“返回的 SearchReply 将有 cursor id 0”与方法签名不符,应改为 AggregationReply 的游标语义(Optional 为空或 Cursor#cursorId 为 0)。
- * 当游标耗尽时,返回的 {@link SearchReply} 将具有 0 的游标 ID。 + * 当游标耗尽时,返回的 {@link AggregationReply} 将不再携带有效游标({@code getCursor().isEmpty()} 或 {@code cursorId == 0})。
🧹 Nitpick comments (17)
src/main/java/io/lettuce/core/cluster/AsyncClusterConnectionProvider.java (1)
22-28: 新增抽象方法可能破坏内部实现;建议提供默认实现或确认所有实现已更新,并补充 since为降低升级风险,可考虑提供默认实现(抛出
UnsupportedOperationException或委托现有逻辑)并补充@since:- <K, V> CompletableFuture<StatefulRedisConnection<K, V>> getRandomConnectionAsync(ConnectionIntent connectionIntent); + /** + * @since 6.8 + */ + default <K, V> CompletableFuture<StatefulRedisConnection<K, V>> getRandomConnectionAsync( + ConnectionIntent connectionIntent) { + throw new UnsupportedOperationException("Not implemented"); + }若不采用默认实现,请确认(并在 PR 说明中注明)所有实现类(如 PooledClusterConnectionProvider 等)均已同步覆盖,否则将导致运行/编译失败。
Also applies to: 40-53
src/test/java/io/lettuce/core/search/RediSearchKeylessRoutingIntegrationTests.java (1)
196-206: 依赖 CommandLatencyCollector 的测试需显式假设开启度量以避免误报当资源中未启用
CommandLatencyCollector时,observedNodeIdsFor(...)将返回空集,当前assertThat(nodes).isNotEmpty()会导致误报。建议在依赖该度量的测试开头添加前置假设:@@ - connection.setReadFrom(ReadFrom.REPLICA); + assumeTrue(clusterClient.getResources().commandLatencyRecorder() instanceof CommandLatencyCollector, + "latency collector not enabled"); + connection.setReadFrom(ReadFrom.REPLICA);同样地,应用于以下测试:
- list_routesRandomly_acrossUpstreams_whenReadFromUpstream
- list_routesRandomly_acrossReplicas_whenReadFromAnyReplica
- search_routesRandomly_acrossUpstreams_whenReadFromUpstream
- search_routesRandomly_acrossReplicas_whenReadFromAnyReplica
这样在未启用命令时延采集的环境下自动跳过相关断言,减少 CI 波动。
Also applies to: 221-229, 242-251, 263-271, 283-291
src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java (1)
1710-1716: ftCursorread(index, cursor, count):参数校验与委托路径清晰,负值 count 作为“未指定”哨兵值也符合现有风格。建议补充 Javadoc 说明 -1 语义,避免调用方误用。src/test/java/io/lettuce/core/search/RediSearchClusterCursorIntegrationTests.java (1)
61-64: 构造器参数注入上的 @Inject 多余且可能引发混淆。JUnit 5 + ParameterResolver(LettuceExtension)已支持按类型解析,无需 javax.inject 注解;保持无注解构造器即可。请确认 CI 所用测试引擎是否依赖 @Inject;若否,建议去除以减少耦合。
src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImplTest.java (1)
64-78: addPartition 后再次调用 updateCache 可能冗余(多数实现会在 addPartition 内更新缓存)。若非必要,建议移除以降低对内部 API 的耦合。src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImplTest.java (2)
139-146: Mockito 参数匹配器尽量指定类型,避免原始any()造成歧义例如:
- when(nodeAsync.ftCursorread(anyString(), any(), anyInt())) + when(((io.lettuce.core.api.async.RediSearchAsyncCommands<String,String>) nodeAsync) + .ftCursorread(anyString(), any(io.lettuce.core.search.AggregationReply.Cursor.class), anyInt()))可提升可读性并减少泛型擦除下的匹配误报。
Also applies to: 150-156
125-131: 异常消息断言与实现保持一致性断言期望包含“missing nodeId”。请确认被测实现确实抛出包含相同文案的
IllegalArgumentException,否则建议断言异常类型并弱化对消息字面值的依赖,或同步实现的异常消息。src/main/java/io/lettuce/core/api/async/RediSearchAsyncCommands.java (1)
1159-1161: Cursor 化 API 设计合理,但属于二进制破坏变更,请补充迁移指引将
long cursorId改为Cursor对外接口有破坏性。建议:
- 在发行说明/迁移指南中给出从
cursorId到Cursor的迁移示例;- 明确
Cursor的来源(来自AggregationReply#getCursor())及空场景处理。Also applies to: 1191-1193, 1230-1232
src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (1)
1160-1162: Cursor‑化 API 一致性良好,建议在文档中补充迁移提示建议在 KDoc “See also/Notes” 增补:Cursor 来源于
AggregationReply.getCursor(),当未创建游标时需要判空处理。Also applies to: 1192-1194, 1232-1233
src/test/java/io/lettuce/test/LoggingTestUtils.java (3)
58-75: attachAppenderFor 使用 Level.ALL 作为阈值可能产生意外过滤;建议传 null 继承 Logger 自身级别传入 Level.ALL 会改变附加器的过滤行为。若希望“不过滤”,应当向 LoggerConfig.addAppender 传 null,让其继承 Logger 的级别。
- Level effectiveLevel = (level != null ? level : Level.ALL); + Level effectiveLevel = level; // 为空时继承 LoggerConfig 自身级别 ... - lc.addAppender(app, effectiveLevel, null); + lc.addAppender(app, effectiveLevel, null);说明:当 level 为 null 时,effectiveLevel 为 null,log4j2 将按 LoggerConfig 的级别分发到该 Appender。
25-42: 避免暴露可变集合;返回只读视图更安全对外公开可变 List 容易被误改。建议将字段改为 private,并在访问器中返回不可变视图。
- public final List<String> messages = new CopyOnWriteArrayList<>(); + private final List<String> messages = new CopyOnWriteArrayList<>(); @@ - public List<String> messages() { - return messages; - } + public List<String> messages() { + return java.util.Collections.unmodifiableList(messages); + }
87-91: computeCallerName 的 startsWith 匹配可能误命中父/子类更精确地匹配调用类可减少重名误判,建议使用 equals 判断。
- return Arrays.stream(st).filter(ste -> ste.getClassName().startsWith(clazz.getName())).findFirst() + return Arrays.stream(st).filter(ste -> ste.getClassName().equals(clazz.getName())).findFirst() .map(ste -> ste.getClassName() + "#" + ste.getMethodName()).orElse("Appender-" + System.nanoTime());src/main/java/io/lettuce/core/search/AggregationReply.java (1)
53-54: 不可变性与健壮性小改进
- replies 可设为 final,避免被重新赋值。
- setCursor 建议显式拒绝 null,避免无效调用被静默忽略。
- List<SearchReply<K, V>> replies = new ArrayList<>(); + final List<SearchReply<K, V>> replies = new ArrayList<>(); @@ - public void setCursor(Cursor cursor) { - if (this.cursor == null) { - this.cursor = cursor; - } - } + public void setCursor(Cursor cursor) { + if (cursor == null) return; + if (this.cursor == null) { + this.cursor = cursor; + } + }Also applies to: 149-151
src/main/java/io/lettuce/core/cluster/api/sync/NodeSelectionSearchCommands.java (1)
1147-1161: 参数文档建议强调非空与来源cursor 参数必须是先前 FT.AGGREGATE/FT.CURSOR READ 返回的游标对象,且非空;建议在 Javadoc 中明确,以避免传入空或手工构造的“游标 id”。
- * @param cursor the cursor obtained from a previous {@code FT.AGGREGATE} or {@code FT.CURSOR READ} command + * @param cursor 非空,由先前 {@code FT.AGGREGATE WITHCURSOR} 或 {@code FT.CURSOR READ} 返回的游标对象Also applies to: 1180-1193, 1218-1231
src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java (2)
873-906: cursor 路由校验良好但异常信息建议更明确报错信息对“cluster mode”有假设。若在单机或路由失败场景,提示可包含 nodeId 缺失与建议(例如先用 ftAggregate WITHCURSOR 获取带 nodeId 的游标)。
- failed.completeExceptionally( - new IllegalArgumentException("Cursor missing nodeId; cannot route cursor READ in cluster mode")); + failed.completeExceptionally(new IllegalArgumentException( + "Cursor missing nodeId; cannot route FT.CURSOR READ. Ensure the cursor was obtained from FT.AGGREGATE WITHCURSOR on this cluster."));
997-1006: READ/WRITE 判定通过 ReadOnlyCommands 探测是对的;建议在异常时降级为 READ 而非一律 WRITE异常时一律回退到 WRITE 可能错过只读从节点,降低吞吐。更稳妥是默认 READ,再在回退 superCall 兜底。
- logger.error("Error while determining connection intent for " + commandType, e); - return ConnectionIntent.WRITE; + logger.error("Error while determining connection intent for " + commandType, e); + return ConnectionIntent.READ;src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java (1)
795-805: READ/WRITE 判定异常时的默认意图建议为 READ与异步实现一致,异常回退 READ 更保守。
- logger.error("Error while determining connection intent for " + commandType, e); - return ConnectionIntent.WRITE; + logger.error("Error while determining connection intent for " + commandType, e); + return ConnectionIntent.READ;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
.github/spellcheck-settings.yml(0 hunks).github/wordlist.txt(0 hunks).github/workflows/spellcheck.yml(0 hunks)src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java(2 hunks)src/main/java/io/lettuce/core/AbstractRedisReactiveCommands.java(3 hunks)src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java(1 hunks)src/main/java/io/lettuce/core/api/StatefulConnection.java(2 hunks)src/main/java/io/lettuce/core/api/async/RediSearchAsyncCommands.java(5 hunks)src/main/java/io/lettuce/core/api/reactive/RediSearchReactiveCommands.java(5 hunks)src/main/java/io/lettuce/core/api/sync/RediSearchCommands.java(5 hunks)src/main/java/io/lettuce/core/cluster/AsyncClusterConnectionProvider.java(2 hunks)src/main/java/io/lettuce/core/cluster/PooledClusterConnectionProvider.java(1 hunks)src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java(6 hunks)src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java(5 hunks)src/main/java/io/lettuce/core/cluster/StatefulRedisClusterConnectionImpl.java(1 hunks)src/main/java/io/lettuce/core/cluster/api/async/NodeSelectionSearchAsyncCommands.java(5 hunks)src/main/java/io/lettuce/core/cluster/api/sync/NodeSelectionSearchCommands.java(5 hunks)src/main/java/io/lettuce/core/masterslave/MasterSlaveConnectionWrapper.java(2 hunks)src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java(1 hunks)src/main/java/io/lettuce/core/search/AggregateReplyParser.java(2 hunks)src/main/java/io/lettuce/core/search/AggregationReply.java(3 hunks)src/main/java/io/lettuce/core/sentinel/StatefulRedisSentinelConnectionImpl.java(1 hunks)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt(5 hunks)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt(2 hunks)src/main/templates/io/lettuce/core/api/RediSearchCommands.java(4 hunks)src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java(2 hunks)src/test/java/io/lettuce/core/cluster/PooledClusterConnectionProviderUnitTests.java(1 hunks)src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImplTest.java(1 hunks)src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImplTest.java(1 hunks)src/test/java/io/lettuce/core/search/RediSearchAggregateIntegrationTests.java(8 hunks)src/test/java/io/lettuce/core/search/RediSearchClusterCursorIntegrationTests.java(1 hunks)src/test/java/io/lettuce/core/search/RediSearchClusterIntegrationTests.java(1 hunks)src/test/java/io/lettuce/core/search/RediSearchKeylessRoutingIntegrationTests.java(1 hunks)src/test/java/io/lettuce/test/LoggingTestUtils.java(1 hunks)src/test/jmh/io/lettuce/core/EmptyStatefulRedisConnection.java(2 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/spellcheck.yml
- .github/spellcheck-settings.yml
- .github/wordlist.txt
🧰 Additional context used
🧬 Code graph analysis (21)
src/test/java/io/lettuce/core/search/RediSearchClusterIntegrationTests.java (2)
src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (2)
ftCursorread(1160-1161)ftCursorread(1192-1193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (2)
ftCursorread(85-86)ftCursorread(88-89)
src/main/java/io/lettuce/core/cluster/PooledClusterConnectionProvider.java (1)
src/main/java/io/lettuce/core/cluster/SlotHash.java (1)
SlotHash(19-151)
src/test/java/io/lettuce/core/cluster/PooledClusterConnectionProviderUnitTests.java (1)
src/main/java/io/lettuce/core/codec/StringCodec.java (1)
StringCodec(44-227)
src/main/java/io/lettuce/core/search/AggregateReplyParser.java (1)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)
src/main/java/io/lettuce/core/api/reactive/RediSearchReactiveCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (5)
ftAggregate(1071-1072)ftAggregate(1127-1128)ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (5)
ftAggregate(79-80)ftAggregate(82-83)ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/test/java/io/lettuce/core/search/RediSearchAggregateIntegrationTests.java (3)
src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (3)
ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (3)
ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)
src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImplTest.java (4)
src/main/java/io/lettuce/core/cluster/models/partitions/Partitions.java (1)
Partitions(63-467)src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/test/java/io/lettuce/test/LoggingTestUtils.java (1)
LoggingTestUtils(18-93)src/main/java/io/lettuce/core/search/arguments/AggregateArgs.java (1)
WithCursor(606-638)
src/main/java/io/lettuce/core/AbstractRedisReactiveCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (3)
ftCursordel(1231-1232)ftCursorread(1160-1161)ftCursorread(1192-1193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (3)
ftCursordel(91-92)ftCursorread(85-86)ftCursorread(88-89)
src/test/java/io/lettuce/core/search/RediSearchKeylessRoutingIntegrationTests.java (3)
src/main/java/io/lettuce/core/ReadFrom.java (1)
ReadFrom(38-271)src/main/java/io/lettuce/core/cluster/models/partitions/Partitions.java (1)
Partitions(63-467)src/main/java/io/lettuce/core/metrics/CommandMetrics.java (1)
CommandMetrics(11-121)
src/main/java/io/lettuce/core/cluster/AsyncClusterConnectionProvider.java (1)
src/main/java/io/lettuce/core/ReadFrom.java (1)
ReadFrom(38-271)
src/main/java/io/lettuce/core/api/sync/RediSearchCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (5)
ftAggregate(1071-1072)ftAggregate(1127-1128)ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (5)
ftAggregate(79-80)ftAggregate(82-83)ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/main/templates/io/lettuce/core/api/RediSearchCommands.java (2)
src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (5)
ftAggregate(1071-1072)ftAggregate(1127-1128)ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (5)
ftAggregate(79-80)ftAggregate(82-83)ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/main/java/io/lettuce/core/cluster/api/async/NodeSelectionSearchAsyncCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (3)
ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (3)
ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (3)
ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (3)
ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java (1)
src/main/java/io/lettuce/core/cluster/ClusterReadOnlyCommands.java (1)
ClusterReadOnlyCommands(17-64)
src/test/java/io/lettuce/core/search/RediSearchClusterCursorIntegrationTests.java (5)
src/test/java/io/lettuce/core/TestSupport.java (1)
TestSupport(33-79)src/main/java/io/lettuce/core/cluster/RedisClusterClient.java (1)
RedisClusterClient(158-1348)src/test/java/io/lettuce/test/LettuceExtension.java (1)
LettuceExtension(85-352)src/test/java/io/lettuce/test/condition/RedisConditions.java (1)
RedisConditions(25-323)src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)
src/main/java/io/lettuce/core/api/async/RediSearchAsyncCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (5)
ftAggregate(1071-1072)ftAggregate(1127-1128)ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (5)
ftAggregate(79-80)ftAggregate(82-83)ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImplTest.java (6)
src/main/java/io/lettuce/core/RedisURI.java (1)
RedisURI(163-1818)src/main/java/io/lettuce/core/cluster/models/partitions/Partitions.java (1)
Partitions(63-467)src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/test/java/io/lettuce/test/LoggingTestUtils.java (1)
LoggingTestUtils(18-93)src/main/java/io/lettuce/core/cluster/SlotHash.java (1)
SlotHash(19-151)src/main/java/io/lettuce/core/search/arguments/AggregateArgs.java (1)
WithCursor(606-638)
src/main/java/io/lettuce/core/cluster/api/sync/NodeSelectionSearchCommands.java (3)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommands.kt (3)
ftCursorread(1160-1161)ftCursorread(1192-1193)ftCursordel(1231-1232)src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (3)
ftCursorread(85-86)ftCursorread(88-89)ftCursordel(91-92)
src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImpl.java (2)
src/main/java/io/lettuce/core/protocol/Command.java (1)
Command(37-186)src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)
src/main/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImpl.java (2)
src/main/java/io/lettuce/core/search/AggregationReply.java (2)
AggregationReply(49-195)Cursor(160-193)src/main/java/io/lettuce/core/search/SearchReply.java (1)
SearchReply(29-292)
🔇 Additional comments (29)
src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java (1)
85-86: 验证 FT_AGGREGATE 和 FT_SEARCH 的只读语义。新增的 8 个 RediSearch 命令已正确添加到只读命令集合中。但需要注意,FT_AGGREGATE 和 FT_SEARCH 在使用 WITHCURSOR 选项时会在服务器端创建游标状态。
请确认:
- 这些命令在集群环境下的路由策略是否正确处理了游标的粘性路由需求
- 游标状态不会影响命令的只读分类(因为游标是临时的、会话级的状态)
从相关代码片段可以看出,Cursor 类型已经包含了 nodeId 用于集群粘性路由,这很好地支持了有状态的游标操作。
根据学习内容
src/main/java/io/lettuce/core/cluster/StatefulRedisClusterConnectionImpl.java (1)
129-132: LGTM!暴露 codec 访问器。新增的
getCodec()方法正确地暴露了内部 codec 字段,与其他连接实现(StatefulRedisConnectionImpl、StatefulRedisSentinelConnectionImpl)保持一致。这是统一 API 表面的一部分。src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java (2)
23-23: LGTM!测试预期已正确更新。从 92 个命令增加到 100 个命令的预期值正确反映了新增的 8 个 RediSearch 命令。
32-32: 检验结果:双下划线标准化是必要且正确的修复您的疑问基于不完整的假设。验证发现:
存在实际的双下划线场景:
CommandType中存在FT_LIST("FT._LIST")命令,其实际字符串包含点和下划线的组合。双下划线如何产生:
"FT._LIST".replace('.', '_')→"FT__LIST"✓- 若无第二次替换,枚举查找会失败
修复的必要性:第二步
replace("__", "_")将"FT__LIST"正确规范化为"FT_LIST",使得CommandType.valueOf("FT_LIST")能够成功匹配。此修改不是防御性编程,而是处理真实边缘情况的必要修复。代码变更是正确且合理的。
Likely an incorrect or invalid review comment.
src/main/java/io/lettuce/core/sentinel/StatefulRedisSentinelConnectionImpl.java (1)
131-134: LGTM!与其他实现保持一致。
getCodec()方法的实现与 StatefulRedisConnectionImpl 和 StatefulRedisClusterConnectionImpl 保持一致,正确暴露了 codec 访问器。src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java (1)
112-112: LGTM!添加 @OverRide 注解。为现有的
getCodec()方法添加@Override注解是良好实践,表明该方法实现了接口契约(可能是 StatefulConnection 接口)。src/main/java/io/lettuce/core/masterslave/MasterSlaveConnectionWrapper.java (1)
138-141: LGTM!正确实现了委托模式。
getCodec()方法正确地委托给底层连接,与此包装器中其他方法的委托模式保持一致。src/test/jmh/io/lettuce/core/EmptyStatefulRedisConnection.java (1)
62-65: LGTM!测试桩实现正确。
getCodec()返回 null 与此测试桩类中其他方法的行为一致(sync()、async()、reactive()、getOptions() 都返回 null)。这是 JMH 测试中的空实现,符合预期。src/main/java/io/lettuce/core/api/reactive/RediSearchReactiveCommands.java (4)
17-17: LGTM!引入 Cursor 类型。新增的
AggregationReply.Cursor导入支持基于 Cursor 的 API 改进,封装了游标 ID 和可选的节点 ID(用于集群粘性路由)。
1164-1194: LGTM!Cursor API 变更一致。
ftCursorread重载方法正确采用了 Cursor 类型,与带 count 参数的版本保持一致。文档清晰说明了当游标耗尽时的行为(cursor id 为 0)。
1196-1233: LGTM!ftCursordel 正确采用 Cursor 类型。
ftCursordel方法签名变更与ftCursorread保持一致,统一使用 Cursor 类型。文档准确描述了游标删除后的行为和最佳实践。
1132-1162: <!-- [scratchpad_update]
[task review_verification 1/1 - continued]
[observations]
- Found Lettuce 6.8.0.RELEASE reference guide showing RediSearch support
- Example code in ref guide shows OLD API: ftCursorread("products-idx", cursorId, 500) with long cursorId (line 1-7)
- This example code directly CONTRADICTS the new method signature in the review (which expects Cursor object)
- GitHub releases page mentions "Introduce RediSearch by @tishun in redis#3375" but no specific API details
- No specific breaking change documentation found for ftCursorread signature change
- Reference guide example shows long cursorId being used, NOT a Cursor object
[analysis]- The reference guide example uses the OLD signature (long cursorId), suggesting it may be outdated OR the API change happened after 6.8 release
- Possible scenarios: (a) documentation is outdated, (b) API was changed post-6.8.0 RELEASE, (c) both signatures exist in transition
- Cannot confirm if the Cursor type change is a documented breaking change or an undocumented evolution
[pending]- Need to check actual source code or look at specific PR redis#3375 that introduced RediSearch
- Cannot conclusively verify if this is a breaking change between 6.8.0 and later or within 6.8
[done]- Search confirmed Lettuce 6.8 RediSearch support exists but API signature details are unclear from documentation
[scratchpad_end] --><!-- [scratchpad_update_2]
[task review_verification - final iteration]
[key_finding]- Reference guide (source 2, indices 12-10) STILL shows OLD API: ftCursorread("products-idx", cursorId, 500) with LONG cursorId
- This is current documentation, not outdated
- The code review shows NEW API with Cursor object parameter
- DIRECT CONTRADICTION found between reference guide and the PR code being reviewed
[critical_observation]- The reference guide example is from official Lettuce documentation, updated recently (search result is current)
- The code snippet in the review shows @experimental marker (line 1157 in review)
- @experimental label from source 12-5 confirms APIs marked @experimental "may undergo API changes in future releases"
[analysis_conclusion]- Cannot definitively confirm if the Cursor type change is officially documented as a breaking change
- Reference guide example uses long cursorId, not Cursor object
- This suggests either: (a) documentation is out of sync with code, (b) API is still evolving, (c) both approaches coexist temporarily
- The review comment correctly identifies this as a breaking change (which it is - parameter type changed)
- However, the review requests verification of version strategy and migration guide - these are NOT clearly documented in the search results
[verification_status]- Confirmed: RediSearch APIs marked @experimental in Lettuce 6.8
- Confirmed: Breaking changes exist in Lettuce 6.8 (deprecated API removals)
- NOT confirmed: Specific ftCursorread signature change is documented in migration guide
- NOT confirmed: Whether this is major version upgrade or minor version with @experimental caveat
- Issue: Reference guide example contradicts the new method signature being reviewed
[decision]- This should be tagged because:
- The specific API change (ftCursorread signature) is not explicitly documented in Lettuce 6.8 breaking changes
- @experimental designation provides some cover for API changes
- Reference guide shows the old signature, suggesting documentation may be outdated
- Cannot conclusively confirm claims about "version strategy" and "migration guide documentation"
[scratchpad_end] -->验证结论:无法完全确认所述的破坏性变更文档和版本策略。
根据网络搜索结果,Lettuce 从 6.8.0.RELEASE 开始支持 Redis Search,且这些命令被标记为 @experimental,可能在未来版本中进行 API 变更。然而,我发现了几个问题:
文档不一致:官方参考指南中的示例代码显示 ftCursorread("products-idx", cursorId, 500),仍使用 long 类型的 cursorId,与代码审查中新的 Cursor 对象参数类型相矛盾。
缺乏明确文档:Web 搜索未发现 ftCursorread 方法签名变更被明确列入 Lettuce 6.8 的正式破坏性变更文档中。
@experimental 标签:虽然 @experimental 注解提供了 API 演变的某种保护,但不足以替代清晰的迁移指南。
建议:
- 确认官方参考指南是否已更新为使用 Cursor 对象,或者两个 API 签名是否共存
- 验证迁移指南中是否有针对此特定方法变更的文档
- 检查 Lettuce 版本号是否确实表示这是主版本升级(通常才会有破坏性变更)
src/test/java/io/lettuce/core/cluster/PooledClusterConnectionProviderUnitTests.java (1)
440-463: 新增用例覆盖合理,LGTM
- 覆盖了写意图下的随机主节点选取与无主时的失败回退通知路径,断言清晰。
- 如 CI 出现偶发失败,可考虑对
join()设置超时或固定随机源以进一步去抖。src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java (2)
53-54: 引入 Cursor 类型合理,保持与新重载方法的签名一致。无异议。
1719-1721: ftCursorread(index, cursor):对上方法的便捷重载,行为一致。无异议。src/test/java/io/lettuce/core/search/RediSearchClusterCursorIntegrationTests.java (5)
80-110: 测试基境搭建完整:版本门槛、防脏数据、索引与样例数据初始化覆盖充分。无异议。
121-151: 同步 API 的游标生命周期与“粘性”校验覆盖到创建、翻页、删除主流程,断言简洁有效。无异议。
153-182: 异步 API 测试等价覆盖良好,使用 join 简化收敛,断言与同步一致。无异议。
184-213: 响应式 API 测试路径齐全,block 用于收敛可接受于集成测试场景。无异议。
215-244: 异常消息与测试断言一致,无需修改验证已完成。实现中的异常消息与测试断言完全一致:
异步实现(RedisAdvancedClusterAsyncCommandsImpl)
- ftCursorread:异常消息
"Cursor missing nodeId; cannot route cursor READ in cluster mode"- ftCursordel:异常消息
"Cursor missing nodeId; cannot route cursor DEL in cluster mode"响应式实现(RedisAdvancedClusterReactiveCommandsImpl)
- 相同的异常消息格式
同步版本:通过 ClusterFutureSyncInvocationHandler 包装异步调用,使用相同的异常
所有版本的异常消息都包含
"missing nodeId",测试中的hasMessageContaining("missing nodeId")断言方式合理且不脆弱。src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterReactiveCommandsImplTest.java (4)
100-117: ftAggregate_stampsNodeId_whenCursorCreated:桩与断言合理,验证了 nodeId 回填行为与游标 id 透传。无异议。
132-147: ftCursorread_routesToNodeId_andStampsNodeId:验证了路由到 WRITE 连接与 nodeId 回填,覆盖关键路径。无异议。
149-158: ftCursordel 路由与返回值断言清晰,并校验了 WRITE 连接的选择。无异议。
168-184: routeKeyless 异常路径:使用失败的 asyncProvider 并断言回退与日志,场景到位。无异议。src/main/java/io/lettuce/core/AbstractRedisReactiveCommands.java (1)
1793-1795: 委托式重载设计合理
ftCursorread(index, cursor)直接委托到带count的重载,语义清晰,保持单一实现点,赞。src/main/kotlin/io/lettuce/core/api/coroutines/RediSearchCoroutinesCommandsImpl.kt (1)
85-92: 协程封装与 Reactive 委托一致,LGTM
awaitFirstOrNull()的使用与返回可空类型匹配良好。src/test/java/io/lettuce/core/cluster/RedisAdvancedClusterAsyncCommandsImplTest.java (2)
86-116: 审查注释不准确 - 无需添加导入代码实际上无需添加
PipelinedRedisFuture的导入。两个类都位于同一包中:
- 测试文件所在包:
io.lettuce.core.clusterPipelinedRedisFuture所在包:io.lettuce.core.clusterJava 中,同包内的类可以直接使用,无需显式导入。代码将正常编译。
Likely an incorrect or invalid review comment.
47-56: 审查评论不正确 -nodeAsync类型完全支持 RediSearch 方法
RedisAsyncCommands<String, String>已经通过继承链包含了所有RediSearchAsyncCommands的方法。在src/main/java/io/lettuce/core/api/async/RedisAsyncCommands.java第42行,该接口明确声明了extends RediSearchAsyncCommands<K, V>。因此,when(nodeAsync.ftAggregate(...))和其他 RediSearch 方法的存根应该能够正常编译而无需任何强制转型或extraInterfaces。Likely an incorrect or invalid review comment.
src/main/java/io/lettuce/core/search/AggregationReply.java (1)
124-137: 审查意见不正确 -AggregationReply#getCursorId()方法从不存在根据代码库验证:
AggregationReply类本身没有getCursorId()方法- 只有内部静态类
AggregationReply.Cursor在第 177 行定义了public long getCursorId()- 所有代码库中的调用都使用
getCursor().get().getCursorId()模式,而不是直接在 AggregationReply 上调用- 不存在对假设的旧方法的调用者
新增的
getCursor()方法并未替换任何直接方法,而是作为访问Cursor对象的新入口点。因此,不存在需要迁移的破坏性变更,建议的弃用桥接方法基于错误的假设。Likely an incorrect or invalid review comment.
| public Mono<String> ftCursordel(String index, Cursor cursor) { | ||
| return createMono(() -> { | ||
| if (cursor == null) { | ||
| throw new IllegalArgumentException("cursor must not be null"); | ||
| } | ||
| long cursorId = cursor.getCursorId(); | ||
| return searchCommandBuilder.ftCursordel(index, cursorId > 0 ? cursorId : 0); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
避免将无效 cursorId=0 发送到服务器,改为前置校验
当前对 cursor.getCursorId() 采用 <=0 ? 0 : id 的回退策略,可能触发 FT.CURSOR DEL 的服务端错误。建议当 cursorId <= 0 时直接抛出参数异常,避免无效请求;同时与项目风格保持一致可使用 LettuceAssert.
建议修改如下:
- return createMono(() -> {
- if (cursor == null) {
- throw new IllegalArgumentException("cursor must not be null");
- }
- long cursorId = cursor.getCursorId();
- return searchCommandBuilder.ftCursordel(index, cursorId > 0 ? cursorId : 0);
- });
+ return createMono(() -> {
+ if (cursor == null) {
+ throw new IllegalArgumentException("cursor must not be null");
+ }
+ long cursorId = cursor.getCursorId();
+ if (cursorId <= 0) {
+ throw new IllegalArgumentException("cursorId must be > 0");
+ }
+ return searchCommandBuilder.ftCursordel(index, cursorId);
+ });🤖 Prompt for AI Agents
In src/main/java/io/lettuce/core/AbstractRedisReactiveCommands.java around lines
1741 to 1748, the code currently converts non-positive cursor IDs to 0 and may
send an invalid FT.CURSOR DEL request; change the precondition to validate the
cursorId and throw an IllegalArgumentException when cursor.getCursorId() <= 0
(use LettuceAssert.notNull for the cursor null check and
LettuceAssert.isTrue(cursorId > 0, "cursorId must be > 0") or equivalent)
instead of mapping to 0 so no invalid cursorId is sent to the server.
|
|
||
| import io.lettuce.core.ClientOptions; | ||
| import io.lettuce.core.RedisConnectionStateListener; | ||
| import io.lettuce.core.codec.RedisCodec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
在公共接口新增方法会造成二进制不兼容;建议提供默认实现并补充 since 标注
向 StatefulConnection 新增抽象方法会破坏所有自定义实现与三方编译产物。建议将 getCodec() 设为默认方法以降低破坏面,并补充 @since。
可按此修改:
- /**
- * @return the {@link RedisCodec} used by this connection.
- */
- RedisCodec<K, V> getCodec();
+ /**
+ * @return the {@link RedisCodec} used by this connection.
+ * @since 6.8
+ */
+ default RedisCodec<K, V> getCodec() {
+ throw new UnsupportedOperationException("getCodec not implemented by this connection");
+ }如确需保持抽象方法,请确认所有实现类均已同步覆盖,否则将导致运行/编译时失败。
Also applies to: 121-125
🏁 Script executed:
#!/bin/bash
# 1. 查找并检查 StatefulConnection.java 中 getCodec() 方法的完整定义
echo "=== 查看 StatefulConnection.java 中的 getCodec() 方法定义 ==="
cat src/main/java/io/lettuce/core/api/StatefulConnection.java | sed -n '115,135p'Length of output: 397
🏁 Script executed:
#!/bin/bash
# 2. 查找所有直接实现 StatefulConnection 接口的类
echo "=== 查找 StatefulConnection 接口的所有实现类 ==="
rg -n "implements.*StatefulConnection" --type=java -A 2 -B 2Length of output: 38
🏁 Script executed:
#!/bin/bash
# 3. 检查主要实现是否已覆盖 getCodec() 方法
echo "=== 检查实现类中 getCodec() 的实现情况 ==="
rg -n "getCodec\s*\(" --type=java -B 3 | head -50Length of output: 3748
🏁 Script executed:
#!/bin/bash
# 4. 查看完整的导入和方法定义区域
echo "=== 查看文件头部导入区域和方法声明 ==="
cat src/main/java/io/lettuce/core/api/StatefulConnection.java | head -135 | tail -30Length of output: 1064
🏁 Script executed:
#!/bin/bash
# 查找所有包含 StatefulConnection 的类定义(更宽泛的搜索)
echo "=== 查找所有可能实现 StatefulConnection 的类 ==="
rg -n "class.*StatefulConnection|implements.*Connection" --type=java | grep -i "statefulconnection\|implements.*Connection" | head -30Length of output: 4860
🏁 Script executed:
#!/bin/bash
# 检查是否在接口中看到 @since 标注
echo "=== 检查 StatefulConnection 接口的完整 Javadoc ==="
cat src/main/java/io/lettuce/core/api/StatefulConnection.java | sed -n '118,126p'Length of output: 193
🏁 Script executed:
#!/bin/bash
# 检查其他模块是否有实现
echo "=== 查找所有 StatefulConnection 接口的直接子类/实现 ==="
ast-grep --pattern 'class $_ implements $$ StatefulConnection $$ {
$$$
}'Length of output: 42
新增抽象方法导致二进制不兼容;需改为默认方法并补充 @SInCE 标注
验证确认存在两个问题:
- 缺少 @SInCE 标注:第121-123行的 Javadoc 中未包含版本信息
- 二进制兼容性破坏:作为抽象方法会强制所有第三方实现重新编译失败
虽然内部实现类(StatefulRedisConnectionImpl、StatefulRedisSentinelConnectionImpl、StatefulRedisClusterConnectionImpl、StatefulRedisPubSubConnectionImpl、StatefulRedisMasterReplicaConnectionImpl、MasterSlaveConnectionWrapper 等)已全部覆盖该方法,但根据提议版本标记 @SInCE 6.8,这应为次版本更新,不宜引入破坏性变更。建议改为默认方法:
/**
* @return the {@link RedisCodec} used by this connection.
+ * @since 6.8
*/
- RedisCodec<K, V> getCodec();
+ default RedisCodec<K, V> getCodec() {
+ throw new UnsupportedOperationException("getCodec not implemented by this connection");
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/io/lettuce/core/api/StatefulConnection.java around line 9 and
Javadoc at lines 121-123, the newly added abstract method breaks binary
compatibility and the Javadoc lacks a @since tag; change the method from
abstract to a default method implementation (delegate to existing behavior or
throw UnsupportedOperationException if appropriate) so existing third-party
implementations remain binary-compatible, and add an appropriate @since 6.8
annotation in the method Javadoc to document the version when it was introduced.
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.Summary by CodeRabbit
发布说明
New Features
Tests
Chores