emecii commented on code in PR #987:
URL: https://github.com/apache/flink-agents/pull/987#discussion_r3744748850
##########
runtime/src/main/java/org/apache/flink/agents/runtime/operator/PythonBridgeManager.java:
##########
@@ -299,14 +300,29 @@ boolean isInitialized() {
@Override
public void close() throws Exception {
- if (pythonActionExecutor != null) {
- pythonActionExecutor.close();
- }
- if (pythonInterpreter != null) {
- pythonInterpreter.close();
+ // Close every component even when an earlier one fails, so a failing
action executor
+ // cannot leak the interpreter or the environment manager. The first
failure is
+ // rethrown with the later ones suppressed.
+ //
+ // The ladder catches Throwable, not Exception, and IOUtils.closeAll
is deliberately not
+ // used: both stop at the first non-Exception Throwable without
closing what follows, and
+ // what follows here is the native Python state.
Review Comment:
Thanks — I had not connected the two beyond the file overlap, and you are
right that the disagreement is on mechanism.
I settled the `Error` case first, as you suggested, and checked it
empirically against `flink-core-2.3.0` rather than reading the source. Three
closeables that record whether `close()` ran, first one throwing:
```
Error -> a1=true a2=false a3=false // remaining two never closed
Exception -> b1=true b2=true b3=true
```
The chain is `closeAll(AutoCloseable...)` -> `closeAll(Iterable)` ->
`closeAll(Iterable, Exception.class)`, and the last one rethrows anything not
assignable to `suppressedException` before closing the rest. So `closeAll`
continues past an `Exception` but stops dead on any non-`Exception`
`Throwable`, exactly as you read it.
That is now recorded on #944 (apache/flink-agents#944 (comment)), including
the consequence at each of its three call sites — its
`ActionExecutionOperator.close()` passes `super::close` as the trailing
closeable, so an `Error` from `resourceCache` skips `stateHandler.dispose()`
too — with a request to adopt the `Throwable` ladder.
On sequencing, my proposal is this PR first, since it is the smaller change
and only rewrites the `close()` methods: #944's rebase then becomes additive,
with `longTermMemory` and `pythonResourceAdapter` slotting into ladders that
already exist, rather than a mechanism swap in one direction or the other. I
have said on #944 that I am equally happy to invert it and rework this as a
follow-up on top of theirs. Deferring to whatever you and @joeyutong prefer —
the part I care about is that the two do not land opposite decisions on the
`Error` case.
--
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]
