Copilot commented on code in PR #6950:
URL: https://github.com/apache/ignite-3/pull/6950#discussion_r2518084199
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -136,6 +136,21 @@ public void cancelBatch() throws InterruptedException {
});
}
+ @Test
+ void resourceNotFoundOnClose() {
+ IgniteSql sql = igniteSql();
+
+ Statement stmt = sql.statementBuilder()
+ .query("SELECT * FROM TABLE(SYSTEM_RANGE(0, 1))") // 2 rows
+ .pageSize(1) // 1 row
+ .build();
+
+ for (int attempt = 0; attempt < 1000; attempt++) {
+ var rs = sql.execute(null, stmt); // preloads 2-nd page in the
background (see SyncResultSetAdapter.IteratorImpl constructor)
+ rs.close(); // Fails with "Failed to find resource"
Review Comment:
The comment "Fails with 'Failed to find resource'" is misleading as it
suggests the test is expected to fail. However, this appears to be a regression
test that should pass after the fix. Consider updating the comment to clarify
that this test verifies the fix for the resource cleanup issue that occurred
when closing a result set without consuming all pages.
```suggestion
rs.close(); // Regression test: verifies that closing a result
set without consuming all pages no longer fails with "Failed to find resource"
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -136,6 +136,21 @@ public void cancelBatch() throws InterruptedException {
});
}
+ @Test
+ void resourceNotFoundOnClose() {
+ IgniteSql sql = igniteSql();
+
+ Statement stmt = sql.statementBuilder()
+ .query("SELECT * FROM TABLE(SYSTEM_RANGE(0, 1))") // 2 rows
+ .pageSize(1) // 1 row
+ .build();
+
+ for (int attempt = 0; attempt < 1000; attempt++) {
+ var rs = sql.execute(null, stmt); // preloads 2-nd page in the
background (see SyncResultSetAdapter.IteratorImpl constructor)
Review Comment:
The comment mentions "preloads 2-nd page in the background" but this is no
longer accurate. The background prefetching has been removed in this PR. The
comment should be updated to reflect that the test verifies that closing a
result set without consuming all pages doesn't cause resource errors.
```suggestion
var rs = sql.execute(null, stmt); // Verifies that closing a
result set without consuming all pages doesn't cause resource errors.
```
--
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]
