github-actions[bot] commented on code in PR #65503:
URL: https://github.com/apache/doris/pull/65503#discussion_r3568361927


##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -314,28 +314,26 @@ public void close() throws IOException {
             if (reader != null) {
                 try {
                     reader.close();
+                    reader = null;
                 } catch (IOException e) {
                     if (exception == null) {
                         exception = e;
                     } else {
                         exception.addSuppressed(e);
                     }
-                } finally {
-                    reader = null;
                 }
             }
             if (ioManager != null) {
                 try {
                     ioManager.close();
+                    ioManager = null;
                 } catch (Exception e) {
                     LOG.warn("Failed to close Paimon JNI IOManager, temp dirs: 
{}", ioManagerTempDirs, e);
                     if (exception == null) {
                         exception = new IOException(e);
                     } else {
                         exception.addSuppressed(e);
                     }
-                } finally {
-                    ioManager = null;
                 }
             }
         } finally {

Review Comment:
   The close retry now preserves the resources that failed to clean up, but 
this `finally` still marks the Java scanner inactive before the close result is 
checked. In the fail-then-retry case added by this PR, `recordIterator`, 
`reader`, and `ioManager` can all remain non-null after the first `close()` 
throws, and C++ keeps the same Java object retryable. However 
`markScannerClosedForMetrics()` flips `scannerCounted` to false and decrements 
`ACTIVE_SCANNERS`, so `gauge:PaimonJniActiveScannerCount` underreports a 
scanner that is still holding resources and awaiting a successful retry. Please 
only mark the scanner closed after all cleanup has succeeded, or restore the 
count on retryable failure, and extend the fail-then-success test to assert the 
active-scanner statistic stays counted until the successful close.
   



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to