PDGGK commented on PR #11750:
URL: https://github.com/apache/seatunnel/pull/11750#issuecomment-5247939710
@DanielLeens thanks — points 2 and 3 were real gaps, and I've pushed 477b2d0
for them.
To be precise about what was and wasn't already covered: the nested
`try/finally` did satisfy your first point (both resources always got a close
attempt), but it silently failed the other two. Java's `finally` semantics mean
an exception from `session.close()` *replaces* the one from the flush, so the
caller saw the teardown symptom and the write error that actually broke the job
was gone.
I followed the shape this repo already uses rather than inventing one —
`connector-pulsar` has exactly this pattern, duplicated in both halves
(`PulsarSinkWriter` L380/L388 and `PulsarSourceReader` L285/L297):
```java
private Throwable appendSuppressed(Throwable existingFailure, Throwable
newFailure) { ... }
private void rethrowCloseFailure(Throwable throwable) throws IOException {
... }
```
Every step now runs unconditionally, the first failure is the one that
propagates, and later ones are attached as suppressed.
**One thing beyond your list.** `Neo4jSourceReader.session` is only assigned
in `open()`, so a reader whose `open()` failed would NPE in `close()` and leak
the driver — the same bug class, on the path where cleanup matters even more.
Guarded, with a test.
**Regression coverage.** `Neo4jCloseTest` goes 2 → 5 cases. Checked against
the previous `try/finally` commit, the three new ones all fail there:
| new case | on the old code |
|---|---|
| sink keeps the flush failure when the session also fails to close | got
`ServiceUnavailableException`, expected the flush's `Neo4jConnectorException` —
**flush failure lost** |
| source keeps the session failure when the driver also fails to close | got
`driver close failed`, expected `session close failed` |
| source closes the driver when `open()` was never called |
`NullPointerException` — driver leaked |
The two original cases pass on both versions, which is right: `try/finally`
did guarantee the close attempts. It was only the failure *identity* it got
wrong.
`mvn test -pl seatunnel-connectors-v2/connector-neo4j` → 7/7 green,
`spotless:check` clean.
--
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]
