fix: stop preparing session on most errors #181
Conversation
googlebot
added
the
cla: yes
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
Most errors that occur during preparing a session for a read/write transaction should be considered permanent, and should stop the automatic preparing of sessions. Any subsequent call to get a read/write session will cause an in-process BeginTransaction RPC to be executed. If the problem has been fixed in the meantime, the RPC will succeed and the automatic prepare of sessions will start again. Otherwise, the error is propagated to the user. Fixes #177
olavloite
force-pushed
the
do-not-retry-exceptions-on-prepare-session
branch
from
388827a
to
3cfe106
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| // Remove the session from the pool. | ||
| if (isClosed()) { | ||
| decrementPendingClosures(1); | ||
| } | ||
| allSessions.remove(session); | ||
| this.resourceNotFoundException = | ||
| MoreObjects.firstNonNull( | ||
| this.resourceNotFoundException, (ResourceNotFoundException) e); |
There was a problem hiding this comment.
In the previous logic, we executed this logic also for PERMISSION_DENIED errors. It doesn't anymore. Is this a problem?
There was a problem hiding this comment.
No, it is still executed for PERMISSION_DENIED errors, but that is now covered by the general error handling that also handles other types of errors. The specific errors DatabaseNotFound and InstanceNotFound are handled specifically, as these should not allow the normal operation of the session pool to be restarted once the error does no longer occur. The reasoning behind that is that a DatabaseNotFound exception can only be fixed by creating a new database with the same name as the old one, but that is still a new database.
The other errors, such as PERMISSION_DENIED and now also for examle FAILED_PRECONDITION, should allow the session pool to restart the automatic preparing of sessions once the error has been fixed. This could be granting the user the necessary permissions or changing the state of database to fulfill all requirements.
| @@ -469,12 +469,25 @@ public void testDatabaseOrInstanceDoesNotExistOnReplenish() throws Exception { | |||
|
|
|||
| @Test | |||
| public void testPermissionDeniedOnPrepareSession() throws Exception { | |||
There was a problem hiding this comment.
This test case covers the PERMISSION_DENIED case discussed above.
|
Thanks for the clarification @olavloite. Please merge when you're ready. |
Most errors that occur during preparing a session for a read/write transaction should be considered permanent, and should stop the automatic preparing of sessions. Any subsequent call to get a read/write session will cause an in-process BeginTransaction RPC to be executed. If the problem has been fixed in the meantime, the RPC will succeed and the automatic prepare of sessions will start again. Otherwise, the error is propagated to the user.
Fixes #177