Update transaction documents #804

brfrn169 · 2023-01-18T14:27:04Z

This PR updates (and improves) the transaction documents. Please take a look!

Torch3333

LGTM, thank you!
I just left a minor suggestion.

docs/api-guide.md

Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

komamitsu · 2023-01-19T01:39:53Z

core/src/main/java/com/scalar/db/api/DistributedTransaction.java

@@ -69,8 +69,8 @@ public interface DistributedTransaction extends TransactionCrudOperable {
  /**
   * Commits a transaction.
   *
-   * @throws CommitConflictException if conflicts happened. You can retry the transaction in this
-   *     case
+   * @throws CommitConflictException if conflicts happened. You can retry the same transaction from


the same transaction sounds the previously failed and rollbacked transaction to me. "the same operations in another transaction" or something would be less confusing?

I googled with "retry the transaction" keyword and it seems very common. So, probably okay. Never mind.

I'm wondering if it could be better to remove same. What do you think?

Suggested change

* @throws CommitConflictException if conflicts happened. You can retry the same transaction from

* @throws CommitConflictException if conflicts happened. You can retry the transaction from

Yeah, retry the transaction doesn't sound the previously failed transaction itself compared to retry the same transaction to me.

Thank you! I'll fix it that way.

@komamitsu Fixed in b512a20. Please take a look again when you have time!

komamitsu · 2023-01-19T02:04:51Z

docs/api-guide.md

-          // Aborting the transaction fails. You can log it here
+          tx.rollback();
+        } catch (RollbackException ex) {
+          // Rolling back the transaction fails. You can log it here


[trivial]

Rolling back the transaction fails

failed sounds a bit better?

Fixed in 1041dda.

komamitsu · 2023-01-19T02:13:03Z

docs/two-phase-commit-transactions.md

+    int retryCount = 0;
+
+    while (true) {
+      if (retryCount > 0) {


[trivial] Incrementing the counter here might be more robust considering future modifications of the exception handling blocks below (e.g. developers notice CrudException should be retried and they change return to break without incrementing the counter)

Fixed in 1041dda.

komamitsu · 2023-01-19T02:14:18Z

docs/two-phase-commit-transactions.md

+
+The APIs for CRUD operations (`get()`/`scan()`/`put()`/`delete()`/`mutate()`) could throw `CrudException` and `CrudConflictException`.
+If you catch `CrudException`, 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 `CrudConflictException`, it indicates a conflict occurs during the transaction so that you can retry the transaction from the beginning, preferably with well-adjusted exponential backoff based on your application and environment.


komamitsu · 2023-01-19T02:15:22Z

docs/two-phase-commit-transactions.md

+    while (true) {
+      if (retryCount > 0) {
+        // Retry the transaction three times maximum in this sample code
+        if (retryCount == 3) {


[trivial] retryCount >= 3 would be a little bit more robust

Fixed in 1041dda.

komamitsu · 2023-01-19T02:19:27Z

docs/two-phase-commit-transactions.md

+If you catch `UnknownTransactionStatusException`, you are not sure if the transaction succeeds or not.
+In such a case, you need to check if the transaction is committed successfully or not and retry it if it fails.
+How to identify a transaction status is delegated to users.
+You may want to create a transaction status table and update it transactionally with other application data so that you can get the status of a transaction from the status table.


I don't really get this. Could you tell me a simple example?

This is explained in the 3rd way in the following article (Japanese):
http://nippondanji.blogspot.com/2009/02/blog-post_23.html

You can have a status (or log) table and insert a record into this table in addition to the usual operations in a transaction. And when you get UnknownTransactionStatusException and want to check if the transaction is really committed, you can check if the record of the status table exists or not. If the record exists, the transaction has been successfully committed.

Great! Thank you for sharing it.

The blog post seems really helpful for our customers as I think it's hard for them to come up with the idea without the details. How about adding a link to similar articles here? I wish if the blog post were written in English...

How about adding a link to similar articles here? I with if the blog post were written in English...

@feeblefakie What do you think? Are there any good articles written in English about that?

BTW (a bit off-topic), do you think it would be great if ScalarDB maintains this kind of transaction history in coordinator.history table or something instead of asking application developers to manage that? ScalarDB error handling can check the table when trying to throw UnknownTransactionStatusException.

How about adding a link to similar articles here?

Sorry, I didn't find good ones...

coordinator.history

Ah, checking coordinator.state might be enough. But the point in this case is that maybe something wrong with the underlying database is happening and it's likely ScalarDB can't access the table... 🤔

[off-topic]

This is explained in the 3rd way in the following article (Japanese):
http://nippondanji.blogspot.com/2009/02/blog-post_23.html

Just remembered I did the same way before.
https://github.com/treasure-data/digdag/blob/42ea7cb71270957ea6424c2df88dd7c65d248789/digdag-standards/src/main/java/io/digdag/standards/operator/redshift/RedshiftConnection.java#L297-L303 The workflow operator for Redshift does CRUD in the user's database and implicitly inserts a status log record to a secret table in the same transaction

Ah, checking coordinator.state might be enough. But the point in this case is that maybe something wrong with the underlying database is happening and it's likely ScalarDB can't access the table... 🤔

Yes, we have coordinator.state which has a state record for each transaction, and we can check the record to see if the transaction is committed or not. And we actually expose it through DistributedTransactionManager.getState().

However, I would like to deprecate (and remove) this API in the future because of the following reasons:

The coordinator.state table is an implementation-specific thing (the Consensus Commit transaction has it, but the JDBC transaction doesn't have it).

We don't want to keep the state records in the coordinator.state table permanently. After successfully committing or rolling back a transaction, we can essentially remove the state record of the transaction (except for the getState() reason). So in the future, I want to remove a state record after the transaction is complete.

We plan to implement the one-phase commit optimization. In this optimization, we don't need to write the state record to the coordinator.state table.

I see. Regarding 1, I don't think we need to expose it to users as far as we can call it internally considering the mode. As for 2 and 3, it would matter. Also, as I mentioned above, automatically internal checking state (or history) table doesn't work when underlying database isn't working fine. So, it's difficult to suggest the best way to handle this kind of situation. I think this PR overall looks good. Probably we can get feedbacks from customers about this section in the future and let's discuss it deeply again.

komamitsu

LGTM! 👍

feeblefakie

Found one typo, but other than that, LGTM! Thank you!

feeblefakie · 2023-01-19T05:16:04Z

core/src/main/java/com/scalar/db/api/DistributedTransaction.java

@@ -69,8 +69,8 @@ public interface DistributedTransaction extends TransactionCrudOperable {
  /**
   * Commits a transaction.
   *
-   * @throws CommitConflictException if conflicts happened. You can retry the transaction in this
-   *     case
+   * @throws CommitConflictException if a transaction conflict occurs, You can retry the transaction


Suggested change

* @throws CommitConflictException if a transaction conflict occurs, You can retry the transaction

* @throws CommitConflictException if a transaction conflict occurs. You can retry the transaction

Ah it's not typo actually. The format @throws XXXException if ... is frequently used in this project.

Update transaction documents

d437a34

brfrn169 added the improvement label Jan 18, 2023

brfrn169 requested review from komamitsu, feeblefakie and Torch3333 January 18, 2023 14:27

brfrn169 self-assigned this Jan 18, 2023

Torch3333 approved these changes Jan 19, 2023

View reviewed changes

docs/api-guide.md Outdated Show resolved Hide resolved

brfrn169 and others added 2 commits January 19, 2023 09:47

Update docs/api-guide.md

f7c7df1

Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

Fix

9f50e13

komamitsu reviewed Jan 19, 2023

View reviewed changes

Fixes based on the feedback

b512a20

brfrn169 force-pushed the update-transaction-documents branch from 679674b to b512a20 Compare January 19, 2023 02:12

brfrn169 requested a review from komamitsu January 19, 2023 02:13

komamitsu reviewed Jan 19, 2023

View reviewed changes

Fixes based on the feedback

1041dda

brfrn169 requested a review from komamitsu January 19, 2023 04:50

komamitsu approved these changes Jan 19, 2023

View reviewed changes

feeblefakie approved these changes Jan 19, 2023

View reviewed changes

Merge branch 'master' into update-transaction-documents

dc6d74e

brfrn169 merged commit 5318515 into master Jan 19, 2023

brfrn169 deleted the update-transaction-documents branch January 19, 2023 07:15

brfrn169 added this to Done in 3.9.0 Apr 27, 2023

brfrn169 added a commit that referenced this pull request Apr 27, 2023

Update transaction documents (#804)

5559c4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update transaction documents #804

Update transaction documents #804

brfrn169 commented Jan 18, 2023 •

edited

Torch3333 left a comment

komamitsu Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023 •

edited

komamitsu Jan 19, 2023

brfrn169 Jan 19, 2023

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

komamitsu Jan 19, 2023

komamitsu Jan 19, 2023

komamitsu Jan 19, 2023

brfrn169 Jan 19, 2023

komamitsu Jan 19, 2023

komamitsu left a comment

feeblefakie left a comment

feeblefakie Jan 19, 2023

brfrn169 Jan 19, 2023

	* @throws CommitConflictException if conflicts happened. You can retry the same transaction from
	* @throws CommitConflictException if conflicts happened. You can retry the transaction from

	* @throws CommitConflictException if a transaction conflict occurs, You can retry the transaction
	* @throws CommitConflictException if a transaction conflict occurs. You can retry the transaction

Update transaction documents #804

Update transaction documents #804

Conversation

brfrn169 commented Jan 18, 2023 • edited

Torch3333 left a comment

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

brfrn169 Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

brfrn169 Jan 19, 2023 • edited

Choose a reason for hiding this comment

komamitsu Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 commented Jan 18, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited

brfrn169 Jan 19, 2023 •

edited

komamitsu Jan 19, 2023 •

edited