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

Revise document for handling exceptions #932

Merged
merged 6 commits into from Jul 13, 2023

Conversation

brfrn169
Copy link
Collaborator

@brfrn169 brfrn169 commented Jul 7, 2023

This PR revises the document for handling exceptions. Please take a look!

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!

Copy link
Member

@josh-wong josh-wong left a comment

Choose a reason for hiding this comment

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

I've added a few comments and suggestions. PTAL!

docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
If you catch `UnknownTransactionStatusException`, you are not sure if the transaction succeeds or not.
If you catch `CommitException`, it indicates that committing the transaction fails due to transient or nontransient faults. You can try retrying the transaction from the beginning, but the transaction may still fail if the cause is nontranient.
If you catch `CommitConflictException`, it indicates that committing the transaction fails due to transient faults (e.g., a conflict error). You can retry the transaction from the beginning.
If you catch `UnknownTransactionStatusException`, it indicates that you are not sure if the transaction succeeds or not.
Copy link
Member

@josh-wong josh-wong Jul 7, 2023

Choose a reason for hiding this comment

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

Suggested change
If you catch `UnknownTransactionStatusException`, it indicates that you are not sure if the transaction succeeds or not.
If you catch `UnknownTransactionStatusException`, it indicates that you are not sure if the transaction succeeds or not.

Although I edited the above sentence a bit, it is a bit unclear to me. Should it indicates that you are not sure if the transaction succeeds or not be something like it indicates that the status of the transaction, whether it has succeeded or not, is unknown instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Thank you! Fixed in 0b33081.

docs/api-guide.md Outdated Show resolved Hide resolved
docs/api-guide.md Outdated Show resolved Hide resolved
brfrn169 and others added 2 commits July 8, 2023 00:00
Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>
@brfrn169 brfrn169 requested a review from josh-wong July 7, 2023 15:05
Copy link
Member

@josh-wong josh-wong 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!🙇‍♂️

try {
tx.rollback();
transaction.rollback();
Copy link
Contributor

Choose a reason for hiding this comment

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

[trivial] I guess those who read the error handling for rollback might wonder why just logging is okay. Maybe describing the reason that the failures of rollback can be ignored would be helpful for users.

Copy link
Collaborator Author

@brfrn169 brfrn169 Jul 11, 2023

Choose a reason for hiding this comment

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

Thank you. I have added some more explanation in ad4828e. Please take a look when you have time!

@komamitsu komamitsu self-requested a review July 11, 2023 05:46
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!

Copy link
Contributor

@komamitsu komamitsu 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants