Lands with #893, which is where the code below is: it is not on master yet.
Problem
JDBCStorage.newCatalogConnection() retries its connect the way CachedConnection.getConnection() retries a borrow, and it does so by repeating the loop rather than by sharing one. The two now carry the same policy in two files:
CachedConnection.getConnectTimeoutSeconds() / getPoolTimeoutSeconds() / deadlineOf() — shared already,CachedConnection.attemptSeconds(connectTimeout, deadline) — the per-attempt clamp,CachedConnection.isWorthRetrying(e, dialect) — which failures are waited out,backoffMs = min(backoffMs == 0 ? 1 : backoffMs * 2, MAX_BACKOFF_MS) — written out in both,- a stall warning, a deadline check, and a timeout exception built from
safeUrl() + redact().
connectCatalog() repeats the establish-and-set-up half of CachedConnection.connect() besides: a fresh Properties, dialect.bound(...), DriverManager.getConnection(...), setAutoCommit(false), TRANSACTION_READ_COMMITTED, close-on-failure, and the lift of the read bound.
Why it matters
The next change to the backoff schedule, to the set of states worth retrying, or to the redaction rules has to be found and repeated by hand, and the pool is the copy anyone will find first. Two differences are deliberate today and nothing marks them as such: the pool feeds its wait into poll() of the deque while the catalog sleeps, and the pool's timeout carries 08001 where the catalog carries the driver's own state on purpose (saysTheConnectionIsGone() reads class 08 as a dropped connection, which would put a healthy attempt into the replay).
What the fix looks like
A package-visible helper in CachedConnection taking the attempt as a callback — roughly
static Connection retryingConnect(String connectionString, ConnectDialect dialect, String what,
LongFunction<Connection> attempt, Waiter waiter)— with the deque poll as the pool's own step and the two intended differences as parameters rather than as drift. CachedConnection.getConnection() is the one path of this class least worth rewriting casually, which is why this was left out of #893 rather than folded into a round about the connection beside it.
Not a defect today
Both loops are covered: CachedConnectionTestCase for the pool, and CatalogConnectionTestCase (a refusal waited out, one that is not, the deadline, the read-bound path) for the catalog. This is about the two staying the same as they change.
Lands with #893, which is where the code below is: it is not on
masteryet.Problem
JDBCStorage.newCatalogConnection()retries its connect the wayCachedConnection.getConnection()retries a borrow, and it does so by repeating the loop rather than by sharing one. The two now carry the same policy in two files:CachedConnection.getConnectTimeoutSeconds()/getPoolTimeoutSeconds()/deadlineOf()— shared already,CachedConnection.attemptSeconds(connectTimeout, deadline)— the per-attempt clamp,CachedConnection.isWorthRetrying(e, dialect)— which failures are waited out,backoffMs = min(backoffMs == 0 ? 1 : backoffMs * 2, MAX_BACKOFF_MS)— written out in both,safeUrl()+redact().connectCatalog()repeats the establish-and-set-up half ofCachedConnection.connect()besides: a freshProperties,dialect.bound(...),DriverManager.getConnection(...),setAutoCommit(false),TRANSACTION_READ_COMMITTED, close-on-failure, and the lift of the read bound.Why it matters
The next change to the backoff schedule, to the set of states worth retrying, or to the redaction rules has to be found and repeated by hand, and the pool is the copy anyone will find first. Two differences are deliberate today and nothing marks them as such: the pool feeds its wait into
poll()of the deque while the catalog sleeps, and the pool's timeout carries08001where the catalog carries the driver's own state on purpose (saysTheConnectionIsGone()reads class 08 as a dropped connection, which would put a healthy attempt into the replay).What the fix looks like
A package-visible helper in
CachedConnectiontaking the attempt as a callback — roughly— with the deque poll as the pool's own step and the two intended differences as parameters rather than as drift.
CachedConnection.getConnection()is the one path of this class least worth rewriting casually, which is why this was left out of #893 rather than folded into a round about the connection beside it.Not a defect today
Both loops are covered:
CachedConnectionTestCasefor the pool, andCatalogConnectionTestCase(a refusal waited out, one that is not, the deadline, the read-bound path) for the catalog. This is about the two staying the same as they change.