Conversation
…e the verdict decides the retry CachedConnection.isWorthRetrying() decides whether a connect the database refused is waited out or reported as it stands, and stopped its walk of the cause and getNextException() chains at MAX_CHAIN_LENGTH. The count did no termination work - the visited set polls every throwable once - it only answered the link it never reached with "the caller's to see": a 53300, a 57P03 or a dialect-retryable link standing 33rd in the failure reported the connect as permanent on a database that would have taken it a moment later, and on the catalog connect of a write left the backend locked down until a restart. The walk now ends where the chains end, the way the walks of JDBCStorage whose verdict a decision reads have since OpenIdentityPlatform#1004. MAX_CHAIN_LENGTH stays for the two budgets that have a reason to stop and say so - the fail-closed threshold of holdsCredentials() and the rebuild budget of redactedCopy() - and its javadoc names them rather than reading as a bound on every walk. testARetryableLinkPastTheOldBudgetOfTheWalkIsLookedAt borrows through a failure that carries the connection-limit state 33rd in its getNextException() chain and pins the second attempt: it fails on master with the failure reported, and passes here.
5f62889 to
5b82d5d
Compare
|
@maximthomas rebased onto master at The conflict was one hunk of On this head: |
1 participant
Fixes #1076
CachedConnection.isWorthRetrying()is the one question that decides whether a connect the database refused is waited out or reported as it stands, and it walked thegetCause()andgetNextException()chains of the failure toMAX_CHAIN_LENGTH(32). The count did no termination work - thevisitedidentity set polls every throwable once, so the walk ends on its own on any chain a driver can allocate - it only decided early: a 53300, a 57P03 or a dialect-retryable link standing 33rd in the failure was never read, and the answer was not "unknown" butfalse, the answer of a failure that is the caller's to see.Two callers read that
falseand throw on it: the borrow (CachedConnection.java:1550), which then reports a database at its limit or on its way up instead of waiting it out withinPOOL_TIMEOUT_PROPERTY; and the catalog connect inside a write (JDBCStorage.java:2130), where the javadoc of the method itself names the cost - the not-accepting-yet state is "the one JDBCStorage.open() has no second attempt of its own for, so a backend that meets it stays locked down until the server is restarted". This is the #961 shape on the connect road, raised in round 2 of #1004.The change
isWorthRetrying()is bounded byvisitedalone -while (!pending.isEmpty())- the way every walk ofJDBCStoragewhose verdict a decision reads has walked withEVERY_LINKsince [#961] Walk every link of a failure where the verdict decides something #1004. A count can only hide a retryable link, never invent one, so this adds no retry that was not due; it costs a walk of the links the driver has already allocated.MAX_CHAIN_LENGTHstays for the two budgets that have a reason to stop and say so - the fail-closed threshold ofholdsCredentials(), where the cost of the other answer is the password of the backend in the log, and the rebuild budget ofredactedCopy(), whose own truncation is JDBC backend: reported() drops the SQLState of a connection-is-gone link past MAX_CHAIN_LENGTH, so a redacted catalog-connect failure reads false through isConnectionFailure() #1074 - and its javadoc names them rather than reading as a bound on every walk of the class.The count arrived with #876 together with the
visitedset that made it redundant: that review turned the exhaustion ofholdsCredentials()into a fail-closedtrueand gaveisWorthRetrying()the identity set only, with nothing said about the count it left answeringfalse.Tests
testARetryableLinkPastTheOldBudgetOfTheWalkIsLookedAtdrives a real borrow through the stub driver with a failure that carries the connection-limit state 33rd in itsgetNextException()chain - the shape of mssql-jdbc, which chains every error of one message that way - and pins the second attempt. On master it fails with the failure reported (SQLException: error 1 of the same message, one attempt); here it passes, and makestestTheWholeChainOfTheFailureIsLookedAtbeside it say what its name claims.Rebased onto master at
26defc28ef(#1009, #1010 and #1018 are in), commit5b82d5df1b. On this head:CachedConnectionTestCase117/117 (116 on master + this one),CatalogConnectionTestCase18/18 - the other reader of the walk -StampConnectionTestCase6/6 andJDBCStorageRetryTest113/113,Skipped: 0.