Skip to content

Ignore primary lease loss in maybeGarbageCollectMultiClientState #2585

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

Merged
merged 4 commits into from
Jan 31, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

This ignores the error if the primary lease is somehow lost between updateClientMetadataAndTryBecomePrimary() and maybeGarbageCollectMultiClientState(). There is no special recovery logic needed as the next primary will run the periodic GC.

Fixes #2555

This ignores the error if the primary lease is somehow lost between updateClientMetadataAndTryBecomePrimary() and maybeGarbageCollectMultiClientState(). There is no special recovery logic needed as the next primary will run the periodic GC.

Fixes #2555
@@ -489,7 +488,12 @@ export class IndexedDbPersistence implements Persistence {
).next(() => inactive);
});
}
);
).catch(() => {
// Ignore primary lease violations or any other type of error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth adding why it's OK to ignore all the errors here.

Something along the line where you put in PR description: There is no special recovery logic needed as the next primary will run the periodic GC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe explain why it is OK to ignore errors in the comments as well?

@wu-hui wu-hui assigned schmidt-sebastian and unassigned wu-hui Jan 31, 2020
@schmidt-sebastian
Copy link
Contributor Author

@wilhuff for approval

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Jan 31, 2020
@schmidt-sebastian schmidt-sebastian merged commit c5b79b6 into master Jan 31, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/recover branch January 31, 2020 21:48
@firebase firebase locked and limited conversation to collaborators Mar 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants