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

Improve documents for Handle Exceptions #897

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

brfrn169
Copy link
Collaborator

@brfrn169 brfrn169 commented Jun 6, 2023

Please take a look!

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.

Overall, it looks good, thank you. I left some suggestions.

docs/api-guide.md Outdated Show resolved Hide resolved
docs/two-phase-commit-transactions.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/two-phase-commit-transactions.md Outdated Show resolved Hide resolved
docs/two-phase-commit-transactions.md Outdated Show resolved Hide resolved
docs/two-phase-commit-transactions.md Outdated Show resolved Hide resolved
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.

Overall, looking good.
Left some minor suggestions. PTAL!

@@ -56,9 +56,11 @@ public interface DistributedTransactionManager {
* Begins a new transaction.
*
* @return {@link DistributedTransaction}
* @throws TransactionNotFoundException if beginning the transaction fails, but it's retryable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @throws TransactionNotFoundException if beginning the transaction fails, but it's retryable.
* @throws TransactionNotFoundException if the transaction fails to start.

I removed but it's retryable because I feel it's a bit redundant with the next sentence.
Ditto for the others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think removing but it's retryable may make the statement sound unsure about the difference from the TransactionException. So, how about removing the next sentence?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then, how about something like this to make things clear?

@throws TransactionNotFoundException if the transaction fails to start due to xxx. You can retry the transaction.
@throws TransactionException if the transaction fails to start due to yyy. You cannot retry the transaction.

Can we say something for xxx and yyy?

Copy link
Collaborator Author

@brfrn169 brfrn169 Jun 9, 2023

Choose a reason for hiding this comment

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

Thank you for the suggestion.

Actually, it's hard to say concrete reasons for TransactionNotFoundException because it only occurs in ScalarDB Cluster.

So, how about some thing like the following?

@throws TransactionNotFoundException if the transaction fails to start due to retryable reasons. You can retry the transaction.
@throws TransactionException if the transaction fails to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

@throws TransactionNotFoundException if the transaction fails to start due to transient faults. You can retry the transaction.
@throws TransactionException if the transaction fails to start due to transient or nontransient faults. You can try retrying the transaction, but you may not be able to start the transaction due to nontransient faults.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! Fixed in 59d8433. Please take a look when you have time!

@@ -781,6 +785,10 @@ public class Sample {
}
```

The `begin()` API could throw `TransactionException` and `TransactionNotFoundException`.
If you catch `TransactionException`, it indicates some failure (e.g., database failure and network error) happens during a transaction, so you should cancel the transaction or retry the transaction after the failure/error is fixed.
Copy link
Contributor

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 `TransactionException`, it indicates some failure (e.g., database failure and network error) happens during a transaction, so you should cancel the transaction or retry the transaction after the failure/error is fixed.
If you catch `TransactionException`, it indicates some failure (e.g., database failure and network error) happens during the transaction, so you should cancel the transaction or retry the transaction after the failure or error is fixed.

Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>
@brfrn169 brfrn169 requested a review from Torch3333 June 7, 2023 04:10
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! 👍

@@ -781,19 +785,29 @@ public class Sample {
}
```

The `begin()` API could throw `TransactionException` and `TransactionNotFoundException`.
If you catch `TransactionException`, it indicates some failure (e.g., database failure and network error) happens during the transaction, so you should cancel the transaction or retry the transaction after the failure or error is fixed.
If you catch `TransactionNotFoundException`, it indicates the transaction is not found, but it's retryable, so you can retry the transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

it indicates the transaction is not found

[minor] Is it possible to describe possible causes of the exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, this doesn't happens with ScalarDB, but It can happen with ScalarDB Cluster. So, I can't describe possible causes here... :(

@brfrn169 brfrn169 requested a review from feeblefakie June 8, 2023 04:48
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.

Left one suggestion. PTAL!

@@ -56,9 +56,11 @@ public interface DistributedTransactionManager {
* Begins a new transaction.
*
* @return {@link DistributedTransaction}
* @throws TransactionNotFoundException if beginning the transaction fails, but it's retryable.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

@throws TransactionNotFoundException if the transaction fails to start due to transient faults. You can retry the transaction.
@throws TransactionException if the transaction fails to start due to transient or nontransient faults. You can try retrying the transaction, but you may not be able to start the transaction due to nontransient faults.

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!

@feeblefakie feeblefakie merged commit e4ca22c into master Jun 13, 2023
12 checks passed
@feeblefakie feeblefakie deleted the improve-documents-for-handle-exceptions branch June 13, 2023 04:49
brfrn169 added a commit that referenced this pull request Jul 7, 2023
brfrn169 added a commit that referenced this pull request Jul 21, 2023
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

4 participants