1991santhu opened a new pull request, #11678:
URL: https://github.com/apache/seatunnel/pull/11678
### Purpose of this pull request
Closes the stale-connection failure reported in #6638.
`ConnectionPoolManager` borrows a connection from the Hikari pool once per
queue index and then holds it for the lifetime of the writer:
```java
public Connection getConnection(int index) {
return connectionMap.computeIfAbsent(index, i -> { ...
connectionPool.getConnection() ... });
}
```
Because the connection never goes back to the pool, Hikari's housekeeper
never gets to inspect it, and none of the pool's own liveness settings can
reach it. On a streaming job the gap between writes can run to hours, and the
server or anything on the path can close the socket in the meantime. The cached
connection is then handed back dead, and the failure surfaces as a write error
*after* the record has already been consumed.
That is the shape in #6638 — the driver reports the last packet as received
tens of millions of milliseconds earlier:
```
The last packet successfully received from the server was 55,800,019
milliseconds ago
```
`autoReconnect=true` does not help, because the connection is never
re-borrowed. The issue has been reported independently by five people since
April 2024, and @dawner84 identified the root cause directly in January — the
connection stays `active` and is never validated ("缺乏探测机制").
### The change
The cached connection is validated before it is returned, and replaced and
closed if it is no longer usable.
The check uses the pool's own configuration rather than forming a second
opinion: `connectionTestQuery` when one is set, since it is configured
precisely for drivers whose `isValid` cannot be trusted, and `isValid`
otherwise. The validation timeout comes from
`HikariDataSource.getValidationTimeout()`, falling back to five seconds when it
is unset.
Two things I want to flag rather than have a reviewer find them:
1. **`computeIfAbsent` → `compute` lengthens the critical section.** The
validation now runs inside the map's per-bin lock. Blocking I/O under that lock
is not new — `connectionPool.getConnection()` was already called inside
`computeIfAbsent` — so this makes an existing critical section longer rather
than introducing a new one. I kept it atomic deliberately:
`SimpleJdbcConnectionPoolProviderProxy` binds one `queueIndex` per instance, so
there is no same-key contention, and validating outside the lock would
reintroduce a check-then-act race where two callers each borrow a connection
for one index. That would leak connections, which is the complaint in #6638 in
the first place.
2. **`isConnectionValid()` becomes self-healing.**
`SimpleJdbcConnectionPoolProviderProxy.isConnectionValid()` calls
`getConnection(queueIndex)`, which may now replace a dead connection as a side
effect and then report valid. I think that is the better behaviour for a
liveness check on a held connection, but it is a change in meaning and worth a
maintainer's opinion.
Behaviour on a live connection is unchanged: borrowed once, served from the
cache thereafter.
### Does this PR introduce _any_ user-facing change?
No new or changed configuration option, and no change to the happy path. The
observable difference is that a JDBC sink which previously failed on the first
write after a long idle period now reconnects and succeeds.
### How was this patch tested?
New unit tests in `ConnectionPoolManagerTest` (6 cases), covering both
directions:
| Test | Covers |
|---|---|
| `reusesAConnectionThatIsStillUsable` | live connection borrowed once, then
cached — no regression |
| `replacesAConnectionThatHasGoneStale` | the #6638 case; dead connection
replaced and closed, not leaked |
| `replacesAConnectionThatIsAlreadyClosed` | `isClosed` short-circuits
before `isValid` |
| `usesTheConfiguredTestQueryInsteadOfIsValid` | `connectionTestQuery` is
honoured when configured |
| `replacesTheConnectionWhenTheTestQueryFails` | a throwing test query
replaces rather than propagates |
| `keepsConnectionsSeparatePerQueueIndex` | per-index isolation preserved |
Run on JDK 11:
```
./mvnw -o test -pl seatunnel-connectors-v2/connector-jdbc
-Dtest=ConnectionPoolManagerTest
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0
```
I checked that the tests actually fail without the fix rather than assuming
it. Reverting only `ConnectionPoolManager.java` and keeping the tests:
```
Tests run: 6, Failures: 4, Errors: 0, Skipped: 0
```
The four failures are exactly the cases that exercise replacement; the two
that still pass are the ones asserting preserved behaviour, which is what
should happen.
Full module, same JDK:
```
./mvnw -o test -pl seatunnel-connectors-v2/connector-jdbc
Tests run: 923, Failures: 0, Errors: 4, Skipped: 6
```
The 4 errors are Testcontainers `startContainer` failures because Docker was
not running on my machine; they are unrelated to this change.
### Check list
* [x] If any new Jar binary package adding in your PR, please add License
Notice according [New License
Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md)
— no new dependency
* [x] If necessary, please update the documentation to describe the new
feature. https://github.com/apache/seatunnel/tree/dev/docs — no new option,
nothing to document
* [x] If necessary, please update `incompatible-changes.md` to describe the
incompatibility caused by this PR — no incompatible change
* [x] If you are contributing the connector code, please check that the
following files are updated — not a new connector, no
mapping/dist/label/plugin_config change needed
--
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]
