Revise document for handling exceptions for Two-phase Commit Transactions #934

brfrn169 · 2023-07-11T05:52:59Z

This PR revises the document for handling exceptions for Two-phase Commit Transactions. Please take a look!

…ions

komamitsu · 2023-07-12T05:23:41Z

docs/two-phase-commit-transactions.md

+  // transaction as committed
+  AtomicReference<Exception> exception = new AtomicReference<>();
+  boolean anyMatch =
+      Stream.of(transaction1, transaction2)


With this error handling, I think the commits can be executed in parallel as follows

Stream.of(transaction1, transaction2) .parallel() .anyMatch( ...

Ah, sorry. I found the parallel version code in the next section below.

This code is the sequential version, right? If so, please ignore my comment. But it might be good to mention sequential execution or something here?

I think that has already been mentioned here:
https://github.com/scalar-labs/scalardb/pull/934/files#diff-2677f880f1a9e512f31cce8e12950863ddffd4be49408d310d72d4d73e6122b9R306

What do you think?

Looks good!

komamitsu · 2023-07-12T05:25:47Z

docs/two-phase-commit-transactions.md

+} catch (Exception e) {
+  // Rollback the transaction
+  if (transaction1 != null) {
+    transaction1.rollback();


Wrapping this with try-catch would be safer just in case if rollback() could throw any exception.

Fixed in 635b41d. Thanks.

komamitsu · 2023-07-12T05:48:55Z

docs/two-phase-commit-transactions.md

+    }
+
+    // Otherwise, throw the first exception
+    throw exceptions.get(0);


[minor] This is a very corner case (and unexpected usage), but this line would throw ~~NPE~~ ArrayIndexOutOfBoundsException if transactions is empty?

Ah, that's a very unexpected usage. We could add validation or an assertion to this method, but I think it might be too verbose since it's a sample code. What do you think?

Yeah, I think it's okay to go with current simple one 👍

docs/two-phase-commit-transactions.md

Torch3333

It looks good, thanks. I left two suggestions.

docs/two-phase-commit-transactions.md

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

Torch3333

LGTM, thank you!

josh-wong

I've added some comments and suggestions. PTAL!

docs/two-phase-commit-transactions.md

josh-wong · 2023-07-13T10:57:20Z

docs/two-phase-commit-transactions.md

+Requests in the same gRPC connection always go to the same server in L3/L4 load balancing.
+When using an L7 load balancer, since requests in the same gRPC connection do not necessarily go to the same server, you need to use cookies or similar for routing requests to correct server.
+For example, when you use [Envoy](https://www.envoyproxy.io/), you can use session affinity (sticky session) for gRPC.
+Or you can also use [Bidirectional streaming RPC in gRPC](https://grpc.io/docs/what-is-grpc/core-concepts/#bidirectional-streaming-rpc) since the L7 load balancer distributes requests in the same stream to the same server.


Suggested change

Or you can also use [Bidirectional streaming RPC in gRPC](https://grpc.io/docs/what-is-grpc/core-concepts/#bidirectional-streaming-rpc) since the L7 load balancer distributes requests in the same stream to the same server.

Alternatively, you can use [bidirectional streaming RPC in gRPC](https://grpc.io/docs/what-is-grpc/core-concepts/#bidirectional-streaming-rpc) since the L7 load balancer distributes requests in the same stream to the same server.

josh-wong · 2023-07-13T10:58:09Z

docs/two-phase-commit-transactions.md

+
+Typically, you use a server-side (proxy) load balancer with HTTP/1.1.
+When using an L3/L4 load balancer, you can use the same HTTP connection to send requests in a transaction, which guarantees the requests go to the same server.
+When using an L7 load balancer, since requests in the same HTTP connection do not necessarily go to the same server, you need to use cookies or similar for routing requests to correct server.


Suggested change

When using an L7 load balancer, since requests in the same HTTP connection do not necessarily go to the same server, you need to use cookies or similar for routing requests to correct server.

When using an L7 load balancer, since requests in the same HTTP connection don't necessarily go to the same server, you need to use cookies or similar method to route requests to the correct server.

docs/two-phase-commit-transactions.md

Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

komamitsu

LGTM! 👍

josh-wong

LGTM! Thank you!🙇‍♂️

feeblefakie

Overall, looking good! Thank you!
And, sorry for the late review.
I left some comments and suggestions.

Also, I think the name Two-phase Commit Transactions might be misleading since readers would think normal transactions don't employ the two-phase commit protocol.
Let's discuss this offline.

feeblefakie · 2023-07-18T08:20:16Z

docs/two-phase-commit-transactions.md

@@ -5,6 +5,21 @@ With Two-phase Commit Transactions, you can execute a transaction that spans mul

 This document briefly explains how to execute Two-phase Commit Transactions in ScalarDB.

+## Overview
+
+ScalarDB transactions normally execute in a single transaction manager instance.


Suggested change

ScalarDB transactions normally execute in a single transaction manager instance.

ScalarDB normally executes transactions in a single transaction manager instance with a one-phase commit interface, which we call normal transactions.

I think we should define normal transactions here because it is referred to in the following sentence.
Also, we should probably note about interfaces.

Maybe, it might be more clear to say something like this since normal transactions don't sound like doing two-phase commits.

Although ScalarDB employs a variant of the two-phase commit protocol, ScalarDB normally executes transactions in a single transaction manager instance with a one-phase commit interface, which we call normal transactions.

Also, it might be helpful to have a diagram like this.
https://speakerdeck.com/scalar/scalar-db-universal-transaction-manager?slide=27

I applied the suggestion in 0db8458.

Also, it might be helpful to have a diagram like this.
https://speakerdeck.com/scalar/scalar-db-universal-transaction-manager?slide=27

Regarding this, let's do in another PR.

docs/two-phase-commit-transactions.md

feeblefakie · 2023-07-18T08:43:51Z

docs/two-phase-commit-transactions.md

+Two-phase commit transactions are basically executed by multiple processes/applications (a coordinator and participants).
+However, in this example code, you can see multiple transaction managers (`transactionManager1` and `transactionManager2`) in a single process.
+
+The following example code shows how to handle exceptions in Two-phase Commit Transactions:


Do we have anything to be noted in particular?
(like, If we don't follow the following guide on how to handle exceptions properly, you may face anomalies...)

Fixed in 272d88b. Can you please check it?

docs/two-phase-commit-transactions.md

feeblefakie · 2023-07-18T08:53:50Z

docs/two-phase-commit-transactions.md

+When using an L3/L4 load balancer, you can use the same HTTP connection to send requests in a transaction, which guarantees the requests go to the same server.
+When using an L7 load balancer, since requests in the same HTTP connection don't necessarily go to the same server, you need to use cookies or similar method to route requests to the correct server.
+You can use session affinity (sticky session) in that case.
+


Should we introduce ScalarDB Cluster a bit?

Yes, we should. Let's do that in another PR. Thanks.

docs/two-phase-commit-transactions.md

Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com> Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

brfrn169 · 2023-07-21T02:12:05Z

@feeblefakie As discussed, let's rephrase the terms Two-phase Commit Transactions to (transactions with) Two-phase Commit interface and normal transactions to (transactions with) One-phase Commit interface in a separate PR. Thanks.

josh-wong · 2023-07-21T02:29:38Z

@brfrn169 Just one note about the following:

@feeblefakie As discussed, let's rephrase the terms Two-phase Commit Transactions to (transactions with) Two-phase Commit interface and normal transactions to (transactions with) One-phase Commit interface in a separate PR. Thanks.

If Two-phase Commit interface and One-phase Commit interface aren't proper names (in ScalarDB), we should lowercase them. Most mentions of these are lowercased in Google search results, so it doesn't seem to be a term that is often capitalized.

komamitsu · 2023-07-21T02:54:01Z

normal transactions to (transactions with) One-phase Commit interface

I might be missing something and sorry in advance if this is a silly comment.

If normal transactions means DistributedTransaction in a sense, it sounds a bit weird since ScalarDB's normal transactions are internally 2PC.

komamitsu · 2023-07-21T02:59:24Z

Ah, I missed #934 (comment). Got the point.

(single service and multi service transactions or something might sound a bit less weird to me...?)

brfrn169 · 2023-07-21T05:06:44Z

If Two-phase Commit interface and One-phase Commit interface aren't proper names (in ScalarDB), we should lowercase them. Most mentions of these are lowercased in Google search results, so it doesn't seem to be a term that is often capitalized.

@josh-wong Thank you for the input. Let's proceed with the lowercase notation.

brfrn169 · 2023-07-21T05:11:48Z

@komamitsu Yes, we should clarify that while ScalarDB uses a variant of the two-phase commit protocol (Consensus Commit), it offers two types of interfaces: one-phase commit interface and two-phase commit interface.

feeblefakie

LGTM! Thank you!

…ions (#934)

[skip ci] Change `two-phase commit transactions` to `transactions with a two-phase commit interface` as noted in #934 (comment).

[skip ci] Change `two-phase commit transactions` to `transactions with a two-phase commit interface` as noted in scalar-labs/scalardb#934 (comment).

brfrn169 added the documentation label Jul 11, 2023

brfrn169 self-assigned this Jul 11, 2023

brfrn169 requested review from komamitsu, feeblefakie, Torch3333 and josh-wong July 11, 2023 05:53

brfrn169 force-pushed the revise-document-for-handling-exceptions-for-2pc branch from 7151c8b to b69b3d5 Compare July 11, 2023 14:46

Revise document for handling exceptions for Two-phase Commit Transact…

ee57bae

…ions

brfrn169 force-pushed the revise-document-for-handling-exceptions-for-2pc branch from b69b3d5 to ee57bae Compare July 11, 2023 14:48

komamitsu reviewed Jul 12, 2023

View reviewed changes

docs/two-phase-commit-transactions.md Outdated Show resolved Hide resolved

Torch3333 reviewed Jul 12, 2023

View reviewed changes

docs/two-phase-commit-transactions.md Outdated Show resolved Hide resolved

docs/two-phase-commit-transactions.md Outdated Show resolved Hide resolved

brfrn169 and others added 2 commits July 12, 2023 16:18

Apply suggestions from code review

163bf8d

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

Fix based on feedback

635b41d

brfrn169 requested review from Torch3333 and komamitsu July 12, 2023 07:24

Torch3333 approved these changes Jul 13, 2023

View reviewed changes

josh-wong requested changes Jul 13, 2023

View reviewed changes

brfrn169 and others added 3 commits July 14, 2023 00:08

Apply suggestions from code review

059b755

Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

Apply suggestions from code review

f73e888

Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

Fix based on feedback

9be4d10

brfrn169 requested a review from josh-wong July 13, 2023 15:24

brfrn169 added 2 commits July 14, 2023 11:28

Update docs/two-phase-commit-transactions.md

e46953e

Fix

7697772

komamitsu approved these changes Jul 18, 2023

View reviewed changes

josh-wong approved these changes Jul 18, 2023

View reviewed changes

feeblefakie reviewed Jul 18, 2023

View reviewed changes

Apply suggestions from code review

0db8458

Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com> Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

Fix based on feedback

272d88b

brfrn169 requested a review from feeblefakie July 21, 2023 05:13

feeblefakie approved these changes Jul 23, 2023

View reviewed changes

feeblefakie merged commit 2ee3c39 into master Jul 23, 2023
13 checks passed

feeblefakie deleted the revise-document-for-handling-exceptions-for-2pc branch July 23, 2023 11:23

feeblefakie mentioned this pull request Jul 23, 2023

Backport to branch(3.10) : Revise document for handling exceptions for Two-phase Commit Transactions #972

Merged

github-actions bot pushed a commit that referenced this pull request Jul 23, 2023

Revise document for handling exceptions for Two-phase Commit Transact…

7f5364b

…ions (#934)

feeblefakie mentioned this pull request Jul 23, 2023

Backport to branch(3.9) failed: Revise document for handling exceptions for Two-phase Commit Transactions #973

Closed

brfrn169 added a commit that referenced this pull request Jul 23, 2023

Revise document for handling exceptions for Two-phase Commit Transact…

7e77e4b

…ions (#934)

This was referenced Jul 23, 2023

Backport to branch(3.9): Revise document for handling exceptions for Two-phase Commit Transactions #974

Merged

Revise document for Handle exceptions #975

Merged

josh-wong added a commit that referenced this pull request Sep 27, 2023

Change phrasing around two-phase commit

98c72af

[skip ci] Change `two-phase commit transactions` to `transactions with a two-phase commit interface` as noted in #934 (comment).

josh-wong mentioned this pull request Sep 27, 2023

Revise configuration-related docs #1082

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise document for handling exceptions for Two-phase Commit Transactions #934

Revise document for handling exceptions for Two-phase Commit Transactions #934

brfrn169 commented Jul 11, 2023

komamitsu Jul 12, 2023

komamitsu Jul 12, 2023

brfrn169 Jul 12, 2023

komamitsu Jul 12, 2023

komamitsu Jul 12, 2023

brfrn169 Jul 12, 2023

komamitsu Jul 12, 2023 •

edited

brfrn169 Jul 12, 2023

komamitsu Jul 12, 2023

Torch3333 left a comment

Torch3333 left a comment

josh-wong left a comment

josh-wong Jul 13, 2023

josh-wong Jul 13, 2023

komamitsu left a comment

josh-wong left a comment

feeblefakie left a comment

feeblefakie Jul 18, 2023

brfrn169 Jul 21, 2023

feeblefakie Jul 18, 2023

brfrn169 Jul 21, 2023

feeblefakie Jul 18, 2023

brfrn169 Jul 21, 2023

brfrn169 commented Jul 21, 2023

josh-wong commented Jul 21, 2023

komamitsu commented Jul 21, 2023

komamitsu commented Jul 21, 2023

brfrn169 commented Jul 21, 2023

brfrn169 commented Jul 21, 2023

feeblefakie left a comment

	Or you can also use [Bidirectional streaming RPC in gRPC](https://grpc.io/docs/what-is-grpc/core-concepts/#bidirectional-streaming-rpc) since the L7 load balancer distributes requests in the same stream to the same server.
	Alternatively, you can use [bidirectional streaming RPC in gRPC](https://grpc.io/docs/what-is-grpc/core-concepts/#bidirectional-streaming-rpc) since the L7 load balancer distributes requests in the same stream to the same server.

	When using an L7 load balancer, since requests in the same HTTP connection do not necessarily go to the same server, you need to use cookies or similar for routing requests to correct server.
	When using an L7 load balancer, since requests in the same HTTP connection don't necessarily go to the same server, you need to use cookies or similar method to route requests to the correct server.

	ScalarDB transactions normally execute in a single transaction manager instance.
	ScalarDB normally executes transactions in a single transaction manager instance with a one-phase commit interface, which we call normal transactions.

Revise document for handling exceptions for Two-phase Commit Transactions #934

Revise document for handling exceptions for Two-phase Commit Transactions #934

Conversation

brfrn169 commented Jul 11, 2023

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 Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

josh-wong left a 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

josh-wong 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

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

brfrn169 commented Jul 21, 2023

josh-wong commented Jul 21, 2023

komamitsu commented Jul 21, 2023

komamitsu commented Jul 21, 2023

brfrn169 commented Jul 21, 2023

brfrn169 commented Jul 21, 2023

feeblefakie left a comment

Choose a reason for hiding this comment

komamitsu Jul 12, 2023 •

edited