-
Notifications
You must be signed in to change notification settings - Fork 101
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
[rel-v0.28] Full snapshot lease update retry on failure #747
[rel-v0.28] Full snapshot lease update retry on failure #747
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @anveshreddy18.
Could you update the description to reflect that this PR is a cherry-pick of #711, #720, and #744?
I also feel that reviewing cherry-pick PRs would be easier if the cherry-picks aren't squashed before creating the PR.
That way, each commit can be verified to correspond to the diffs that a particular merged PR to master brought.
It would also maintain the same commit messages (when squashed and merged to this branch) that are present in the master branch, so it is easy to map changes in this branch and master.
@renormalize Out of these three PR's, the first one contains most of the functionality and since the other two PR's were just reverts and don't add anything, it was advised by @ishan16696 to not add those commits. But I agree it would make the reviewer's life easier in general, but in this case, since these are reverts, I didn't want to include them as commits. I hope this is fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified to have the same diff as the three PRs mentioned. Thanks.
Just 1 nit-pick, this PR can be splitted into 2 commits. 1 commit for cherry-pick and another commit for all reverts on top of that cherry-pick. May be I wasn't able to convey it properly. Anyway it look good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
Tested the scenario and it tries to renew the full snapshot lease every 1 min now. |
What this PR does / why we need it:
cherry-pick of PR #711, #720, and #744 combined together
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: