Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stop preparing session on most errors #181

Merged
merged 1 commit into from Apr 28, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Apr 25, 2020

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

@googlebot googlebot added the cla: yes label Apr 25, 2020
@olavloite olavloite added the kokoro:force-run label Apr 25, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 25, 2020
@olavloite olavloite added the kokoro:force-run label Apr 26, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 26, 2020
@olavloite olavloite requested review from skuruppu and hengfengli Apr 26, 2020
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 olavloite force-pushed the do-not-retry-exceptions-on-prepare-session branch from 388827a to 3cfe106 Compare Apr 26, 2020
@olavloite olavloite added the kokoro:force-run label Apr 26, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 26, 2020
// Remove the session from the pool.
if (isClosed()) {
decrementPendingClosures(1);
}
allSessions.remove(session);
this.resourceNotFoundException =
MoreObjects.firstNonNull(
this.resourceNotFoundException, (ResourceNotFoundException) e);
Copy link
Contributor

@skuruppu skuruppu Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous logic, we executed this logic also for PERMISSION_DENIED errors. It doesn't anymore. Is this a problem?

Copy link
Contributor Author

@olavloite olavloite Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor Author

@olavloite olavloite Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case covers the PERMISSION_DENIED case discussed above.

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Apr 28, 2020

Thanks for the clarification @olavloite. Please merge when you're ready.

@olavloite olavloite merged commit d0e3d41 into master Apr 28, 2020
13 checks passed
@olavloite olavloite deleted the do-not-retry-exceptions-on-prepare-session branch Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants