Copilot commented on code in PR #29574:
URL: https://github.com/apache/flink/pull/29574#discussion_r4354286286
##########
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/ForStStateExecutor.java:
##########
@@ -216,14 +215,7 @@ public CompletableFuture<Void> executeBatchRequests(
coordinatorThread)
.exceptionally(
e -> {
- try {
- for (ForStDBIterRequest<?, ?, ?,
?, ?> iterRequest :
- iterRequests) {
- iterRequest.close();
- }
- } catch (IOException ioException) {
- LOG.error("Close iterRequests
fail", ioException);
- }
+ // Iterators are closed only by their
owner.
Review Comment:
Removing the batch cleanup leaves successful partial-page iterators without
a cleanup path when a sibling request fails. If the held request in
`testBatchFailureLeavesIteratorsToTheirOwners` has more than `CACHE_SIZE_LIMIT`
entries, it returns an open iterator after the batch fails. `executionError`
then rejects further loading requests, and neither `ForStMapIterator` nor
executor shutdown closes the retained handle. The worker-side `failRequest`
does not cover this successful request. Add owner-safe reclamation for
outstanding iterators, for example by tracking them and closing them during
shutdown only after all workers have stopped. Keep the cross-thread close loop
removed, and extend the regression test to cover a partial page and eventual
handle cleanup.
--
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]
