weiqingy commented on code in PR #974:
URL: https://github.com/apache/flink-agents/pull/974#discussion_r3742006867
##########
runtime/src/main/java/org/apache/flink/agents/runtime/actionstate/FlussActionStateStore.java:
##########
@@ -490,15 +490,32 @@ public void pruneState(Object key, long seqNum) {
@Override
public void close() throws Exception {
+ Exception firstException = null;
try {
if (table != null) {
table.close();
}
+ } catch (Exception e) {
+ firstException = e;
Review Comment:
You're right on all of it. I reproduced each path against the previous code
before changing anything:
- table `Error` plus connection `Exception`: the `Error` reaches the caller
with `getSuppressed()` empty, so the connection failure is gone.
- table `Exception` plus connection `Error`: the connection `Error` is
thrown and the table exception is lost.
- Kafka producer `Error`: `consumer.close()` is never invoked at all, so
that one is a live resource leak and not only exception loss.
- Kafka producer `Exception` plus consumer `Error`: the producer failure is
lost.
Both methods now use the same shape. Each close gets its own `catch
(Throwable t)`, the first failure stays primary via
`ExceptionUtils.firstOrSuppressed`, and the method ends with
`ExceptionUtils.rethrowException` so an `Error` still arrives as an `Error` and
a checked exception as itself. `rethrowException` is needed rather than a bare
`throw` because the accumulator is `Throwable`-typed and would not otherwise
compile.
I took the Kafka fix here rather than as a follow-up, since its `Error` path
leaks the consumer and deferring it would leave the two stores divergent.
Widening to `Throwable` also removes the reason the `finally` existed, so it
is gone and the two methods are now structurally identical. That supersedes the
design note in the PR description arguing the `finally` had to stay, and I have
rewritten the description accordingly. That argument only ever held for `catch
(Exception)` not running on an `Error`, and it stops holding once both arms
catch `Throwable`.
On the test: `testCloseKeepsTableErrorWhenConnectionCloseAlsoFails` now
asserts the connection failure survives as a suppressed exception, rather than
only that the `Error` stays primary. Each test class covers the same six close
outcomes, and each of the four new or strengthened tests kills a distinct
mutant: narrowing any single one of the four `catch (Throwable)` arms back to
`catch (Exception)` fails exactly one of them and no others.
--
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]
