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

bugfix: only update in memory lease if DDB lease renews without error #1354

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

lucienlu-aws
Copy link
Contributor

@lucienlu-aws lucienlu-aws commented Jun 18, 2024

Issue #, if available:

Description of changes:
There is a bug in DDBLeaseRenewer such that if DDB fails to update, the in memory lease would still be updated. This commit fixes this by reverting the update if the DDB update throws any error

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lucienlu-aws lucienlu-aws force-pushed the bugfix-lease-renewer branch 2 times, most recently from 5766e39 to 482ce71 Compare June 19, 2024 21:25
Copy link
Contributor

@brendan-p-lynch brendan-p-lynch left a comment

Choose a reason for hiding this comment

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

I think this might be too broad and not future proof to exceptions where we wouldn't want to roll back. It seems like there could be a problem where we catch some exception but it did update the table and we roll back in memory anyway.

@@ -358,6 +359,10 @@ public boolean updateLease(
success = true;
return updatedLease;
}
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little bit concerned that we catch this on every exception. Is it not possible that there are exceptions that we would not want to revert the lease on? Shouldn't we only catch this if the DDB update fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also even if this is the case now wouldn't it be better to define them in case another case comes along where the exception woudn't stop the lease from updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your concern, updated to be only exceptions thrown when failing to update DDB

@lucienlu-aws lucienlu-aws force-pushed the bugfix-lease-renewer branch from 482ce71 to 380b8fc Compare June 21, 2024 22:45
Copy link
Contributor

@brendan-p-lynch brendan-p-lynch left a comment

Choose a reason for hiding this comment

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

I see that the backward compatibility tests are failing. Approving but just want to make sure we are confident it is backward compadible before merging.

@lucienlu-aws
Copy link
Contributor Author

Backwards compatibility tests are failing due to previous PR #1340. Will double check with owners

@lucienlu-aws
Copy link
Contributor Author

Double checked with owner of #1340, the failures is due to internally generated code and we should be okay.

@lucienlu-aws lucienlu-aws merged commit 715690d into awslabs:master Jun 24, 2024
1 of 2 checks passed
@lucienlu-aws lucienlu-aws deleted the bugfix-lease-renewer branch June 24, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants