Copilot commented on code in PR #7667:
URL: https://github.com/apache/ignite-3/pull/7667#discussion_r2853161797
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SqlOutdatedPlanTest.java:
##########
@@ -167,13 +178,22 @@ void schemaChangedAndNodeDisconnectedDuringPlanning() {
semaphore.release();
- await(await(fut).closeAsync());
+ AsyncSqlCursor<InternalSqlRow> cursor = await(fut);
// Planning must be repeated, but only once.
assertThat(prepareServiceSpy.callsCounter.get(), is(2));
// The plan execution must be repeated using a new transaction.
assertThat(txContext.startedTxCounter.get(), is(2));
+
+ drainAndCloseCursor(cursor);
Review Comment:
Same cleanup issue here: if an assertion fails before
`drainAndCloseCursor(cursor)` runs, the cursor won't be closed, which can
interfere with `cluster.stop()` and cause flakiness. Consider ensuring cursor
closure in a finally block once `await(fut)` succeeds.
```suggestion
try {
// Planning must be repeated, but only once.
assertThat(prepareServiceSpy.callsCounter.get(), is(2));
// The plan execution must be repeated using a new transaction.
assertThat(txContext.startedTxCounter.get(), is(2));
drainAndCloseCursor(cursor);
} finally {
await(cursor.closeAsync());
}
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SqlOutdatedPlanTest.java:
##########
@@ -167,13 +178,22 @@ void schemaChangedAndNodeDisconnectedDuringPlanning() {
semaphore.release();
- await(await(fut).closeAsync());
+ AsyncSqlCursor<InternalSqlRow> cursor = await(fut);
// Planning must be repeated, but only once.
assertThat(prepareServiceSpy.callsCounter.get(), is(2));
// The plan execution must be repeated using a new transaction.
assertThat(txContext.startedTxCounter.get(), is(2));
+
+ drainAndCloseCursor(cursor);
+ }
+
+ private static void drainAndCloseCursor(AsyncSqlCursor<InternalSqlRow>
cursor) {
+ BatchedResult<InternalSqlRow> result =
await(cursor.requestNextAsync(1024));
+ assertThat(result.items(), is(not(empty())));
+ assertThat(result.hasMore(), is(false));
Review Comment:
`drainAndCloseCursor` assumes a single `requestNextAsync(1024)` will fully
drain the result set and asserts `hasMore()` is false. This makes the test
brittle if the default partition count or data provider changes (or if the
engine returns smaller batches), and the assertions are not required for
ensuring cleanup. Prefer draining in a loop until `hasMore()` is false (or just
close without asserting on `hasMore()`).
```suggestion
boolean sawRows = false;
while (true) {
BatchedResult<InternalSqlRow> result =
await(cursor.requestNextAsync(1024));
if (!result.items().isEmpty()) {
sawRows = true;
}
if (!result.hasMore()) {
break;
}
}
assertThat(sawRows, is(true));
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SqlOutdatedPlanTest.java:
##########
@@ -134,13 +143,15 @@ void planningIsRepeatedUsingTheSameTransaction(TxType
type) {
semaphore2.release();
- await(await(fut).closeAsync());
+ AsyncSqlCursor<InternalSqlRow> cursor = await(fut);
// Planning must be repeated, but only once.
assertThat(prepareServiceSpy.callsCounter.get(), is(2));
// Transaction should be started only once.
assertThat(txContext.startedTxCounter.get(), is(1));
+
+ drainAndCloseCursor(cursor);
}
Review Comment:
`cursor` is only closed via `drainAndCloseCursor(cursor)` after the
assertions. If any assertion fails, the cursor will leak and can make
`cluster.stop()` in `@AfterEach` hang or affect subsequent tests. Wrap the
assertions in a try/finally (or use a utility that always closes) so the cursor
is closed even on failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
