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.

Sign up for GitHub

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

Jump to bottom

Refactoring the callbacks for the group commit error handling in CommitHandler #2390

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Refactoring the callbacks for the group commit error handling in CommitHandler #2390

merged 7 commits into from
Dec 16, 2024

Conversation

komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Dec 5, 2024 •
edited by brfrn169
Loading

Description

In the current implementation, CommitHandler has onPrepareFailure() and onValidateFailure(), but there is no need to separate them. This PR merges them to make it simpler.

Related issues and/or PRs

None.

Changes made

  • Merge onPrepareFailure() and onValidateFailure() into onFailureBeforeCommit()
  • Make the error handling more consistent

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

None

Release notes

N/A

@komamitsu komamitsu changed the title Refactor error handling for the group commit in CommitHandler Refactoring the callbacks for the group commit error handling in CommitHandler Dec 5, 2024
@komamitsu komamitsu self-assigned this Dec 5, 2024
komamitsu
komamitsu commented Dec 5, 2024
View reviewed changes
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java Outdated
@@ -65,11 +69,9 @@ private Optional<Future<Void>> invokeBeforePreparationSnapshotHook(Snapshot snap
return Optional.of(
beforePreparationSnapshotHook.handle(tableMetadataManager, snapshot.getReadWriteSets()));
} catch (Exception e) {
onFailureBeforeCommit(snapshot);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this callback first is safe since it doesn't throw any exception.

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java Outdated
@@ -104,28 +104,30 @@ public void commit(Snapshot snapshot) throws CommitException, UnknownTransaction
try {
prepare(snapshot);
} catch (PreparationException e) {
onFailureBeforeCommit(snapshot);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the following abortState() internally calls a method equivalent to onFailureBeforeCommit() in case of CommitHandlerWithGroupCommit. So, this change doesn't change the behavior at all. But, I think the callback should be called here for consistent error handling.

Torch3333
Torch3333 approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

brfrn169
brfrn169 reviewed Dec 10, 2024
View reviewed changes
core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java Outdated
@@ -65,11 +69,9 @@ private Optional<Future<Void>> invokeBeforePreparationSnapshotHook(Snapshot snap
return Optional.of(
beforePreparationSnapshotHook.handle(tableMetadataManager, snapshot.getReadWriteSets()));
} catch (Exception e) {
onFailureBeforeCommit(snapshot);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For safety, I think we can catch the exception as follow:

Suggested change
onFailureBeforeCommit(snapshot);
try {
onFailureBeforeCommit(snapshot);
} catch (Exception ex) {
...
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you have a point. On the other hand, I think all the codes calling the callback need the same just-in-case error handling in terms of consistency. So, I wrote this comment https://github.com/scalar-labs/scalardb/pull/2390/files#diff-80b71c10ff00c8a739d8aec837a1730b5470df99295bebb354033c9aefc33bbaR55-R56 so that we don't need such a just-in-case error handling. But if you think the comment isn't enough, I'm okay to add the error handling to all the codes calling the callback. Either is fine with me 👍

Copy link
Contributor Author

@komamitsu komamitsu Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@brfrn169 On second thought, I'll follow your suggestion since trusting an overridden callback is a bit too optimistic and the comment doesn't guarantee a callback won't throw an exception. Let me update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it in 782cb03

@komamitsu komamitsu requested a review from brfrn169 December 11, 2024 12:47
brfrn169
brfrn169 approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java
Comment on lines +78 to +79
logger.warn(
"Unexpectedly failed to remove the snapshot ID from the group committer. ID: {}", id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit:

Suggested change
logger.warn(
"Unexpectedly failed to remove the snapshot ID from the group committer. ID: {}", id);
logger.warn(
"Unexpectedly failed to remove the snapshot ID from the group committer. ID: {}", id, e);

feeblefakie
feeblefakie approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@komamitsu komamitsu merged commit 185ca4a into master Dec 16, 2024
48 checks passed
@komamitsu komamitsu deleted the refactor-commit-handler-with-group-commit branch December 16, 2024 02:48
feeblefakie pushed a commit that referenced this pull request Dec 16, 2024
@komamitsu
Refactoring the callbacks for the group commit error handling in `Com…
e5e05bb 
…mitHandler` (#2390)
@feeblefakie feeblefakie mentioned this pull request Dec 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brfrn169 brfrn169 brfrn169 approved these changes

@feeblefakie feeblefakie feeblefakie approved these changes

@Torch3333 Torch3333 Torch3333 approved these changes

Assignees

@komamitsu komamitsu

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@komamitsu @brfrn169 @feeblefakie @Torch3333