Allow directly executing CRUD operations with transaction managers #1755

brfrn169 · 2024-05-21T08:36:50Z

Description

This PR allows directly executing CRUD operations with transaction managers.

Related issues and/or PRs

N/A

Changes made

Added CRUD operation methods (get(), scan(), put(), insert(), upsert(), update(), delete(), and mutate()) to DistributedTransactionManager and TwoPhaseCommitTransactionManager
- Introduced CrudOperable, and made TransactionCrudOperable a subclass of CrudOperable
- Also introduced TransactionManagerCrudOperable as a subclass of CrudOperable
- Made DistributedTransactionManager and TwoPhaseCommitTransactionManager extend TransactionManagerCrudOperable

Checklist

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)

N/A

Release notes

Allowed directly executing CRUD operations with transaction managers.

brfrn169 · 2024-05-21T08:38:04Z

core/src/main/java/com/scalar/db/common/TransactionDecorationDistributedTransactionManager.java

+import java.util.Optional;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+public abstract class TransactionDecorationDistributedTransactionManager


Extracted transaction decoration related logic from AbstractDistributedTransactionManager to TransactionDecorationDistributedTransactionManager.

brfrn169 · 2024-05-21T08:38:33Z

...rc/main/java/com/scalar/db/common/TransactionDecorationTwoPhaseCommitTransactionManager.java

+import java.util.Optional;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+public abstract class TransactionDecorationTwoPhaseCommitTransactionManager


Extracted transaction decoration related logic from AbstractTwoPhaseCommitTransactionManager to TransactionDecorationTwoPhaseCommitTransactionManager.

brfrn169 · 2024-05-21T08:41:00Z

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

+public interface DistributedTransactionManager
+    extends TransactionManagerCrudOperable, AutoCloseable {


Made DistributedTransactionManager extends TransactionManagerCrudOperable.

brfrn169 · 2024-05-21T08:41:21Z

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

+public interface TwoPhaseCommitTransactionManager
+    extends TransactionManagerCrudOperable, AutoCloseable {


Also made TwoPhaseCommitTransactionManager extend TransactionManagerCrudOperable.

brfrn169 · 2024-05-21T08:42:01Z

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

+ * always used in transactional CRUD operations, so {@link Consistency} specified for CRUD
+ * operations is ignored.
+ */
+public interface CrudOperable<E extends TransactionException> {


Introduce CrudOperable that's an abstraction for executing CRUD operations.

brfrn169 · 2024-05-21T08:42:54Z

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

+import java.util.Optional;
+
+/** An interface for transactional CRUD operations for transaction managers. */
+public interface TransactionManagerCrudOperable extends CrudOperable<TransactionException> {


TransactionManagerCrudOperable is an abstraction for transaction managers to execute CRUD operations.

komamitsu · 2024-05-22T02:40:45Z

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

- */
-public interface TransactionCrudOperable {
+/** An interface for transactional CRUD operations for transactions. */
+public interface TransactionCrudOperable extends CrudOperable<CrudException> {


This might be just a naming issue. IIUC, CrudException inherits from TransactionException, and TransactionCrudOperable inherits from CrudOperable. Their inheritance directions seem reversed from the perspective of class names. It's a bit confusing?

Good point. Thanks.

Actually, this naming strongly depends on backward compatibility. TransactionCrudOperable is an existing name, so we can't rename it to preserve backward compatibility.

The requirements here are as follows:

Add CRUD operation methods to the transaction manager interfaces (DistributedTransactionManager and TwoPhaseCommitTransactionManager).

Access the CRUD operation methods through the same abstraction for the transaction manager interfaces and the transaction interfaces (DistributedTransaction and TwoPhaseCommitTransaction).

The exceptions thrown by the CRUD operation methods differ slightly between the transaction manager interfaces and the transaction interfaces.

Considering these requirements, we finally decided on the current naming. But do you have any better ideas for the naming?

Thanks for sharing the background.

What we can change is only the name of CrudOperable newly added in this PR. It seems to be only inherited by TransactionManagerCrudOperable and TransactionCrudOperable. How about renaming it to TransactionCrudOperableBase or something so that the inheritance relationships will be as follows?

TransactionCrudOperableBase

TransactionCrudOperable

TransactionManagerCrudOperable

TransactionException

CrudException

Current

CrudOperable

TransactionCrudOperable

TransactionManagerCrudOperable

TransactionException

CrudException

Actually, TransactionManagerCrudOperable is also newly added in this PR, so we can rename it as well.

The naming XXXBase sounds more appropriate for abstract classes to me, so I'm not sure it's suitable for interfaces. What do you think?

I see. I think TransactionManagerCrudOperable is alined with TransactionCrudOperable, so it can be left as is. Regarding XxxxBase, indeed it's less common. I'm also not sure if we should avoid it. Another option is TransactionalCrudOperable based on the comment An interface for transactional CRUD operations? It might be a bit confusing, though.

Yeah, TransactionalCrudOperable is a possible idea. But, as you mentioned, it might still be confusing between TransactionalCrudOperable and TransactionCrudOperable.

If we consider CrudOperable vs. TransactionalCrudOperable, I prefer CrudOperable. But that's just my preference. If you have any other better ideas, strong opinions, or strong reasons, please let me know. Thanks.

komamitsu · 2024-05-22T04:01:15Z

core/src/main/java/com/scalar/db/common/AbstractDistributedTransactionManager.java

+    try {
+      transaction.rollback();
+    } catch (RollbackException e) {
+      throw new CrudException(e.getMessage(), e, e.getTransactionId().orElse(null));


Uncommitted transaction will be rolled back, so just warning message or something is enough here?

Correct. Let me add a warning message here. Thanks.

Fixed in 64417d1. Thanks.

komamitsu · 2024-05-22T04:20:17Z

...rc/main/java/com/scalar/db/common/TransactionDecorationTwoPhaseCommitTransactionManager.java

+  static class StateManagedTransaction extends DecoratedTwoPhaseCommitTransaction {
+
+    private enum Status {
+      ACTIVE,


[minor] This is a bit off topic. But PREPARED and VALIDATED may seem active as well. So, EXECUTING or CRUD or something might be less confusing?

Yes, you are right. Let's discuss this in a separate PR. Thanks.

Torch3333

Besides this comment, LGTM, thank you.

Torch3333 · 2024-05-23T06:07:56Z

core/src/main/java/com/scalar/db/common/AbstractTwoPhaseCommitTransactionManager.java

+    implements TwoPhaseCommitTransactionManager {
+
+  private static final Logger logger =
+      LoggerFactory.getLogger(AbstractDistributedTransactionManager.class);


Suggested change

LoggerFactory.getLogger(AbstractDistributedTransactionManager.class);

LoggerFactory.getLogger(AbstractTwoPhaseCommitTransactionManager.class);

Good catch! Fixed in 194e18d. Thanks.

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

…tion-managers

…1755)

brfrn169 added the enhancement New feature or request label May 21, 2024

brfrn169 self-assigned this May 21, 2024

brfrn169 commented May 21, 2024

View reviewed changes

Allow directly executing CRUD operations with transaction managers

d6f9cca

brfrn169 force-pushed the allow-directly-executing-crud-with-transaction-managers branch from bf10799 to d6f9cca Compare May 21, 2024 13:41

brfrn169 requested review from komamitsu, feeblefakie and Torch3333 May 22, 2024 00:08

komamitsu reviewed May 22, 2024

View reviewed changes

Fix based on feedback

64417d1

brfrn169 force-pushed the allow-directly-executing-crud-with-transaction-managers branch from 0b1c67c to 64417d1 Compare May 23, 2024 02:29

brfrn169 requested a review from komamitsu May 23, 2024 02:30

Torch3333 approved these changes May 23, 2024

View reviewed changes

Fix based on feedback

194e18d

komamitsu approved these changes May 27, 2024

View reviewed changes

feeblefakie approved these changes May 27, 2024

View reviewed changes

Merge branch 'master' into allow-directly-executing-crud-with-transac…

b238738

…tion-managers

brfrn169 merged commit ab818c6 into master May 27, 2024
24 checks passed

brfrn169 deleted the allow-directly-executing-crud-with-transaction-managers branch May 27, 2024 05:24

feeblefakie pushed a commit that referenced this pull request May 27, 2024

Allow directly executing CRUD operations with transaction managers (#…

136c3bf

…1755)

feeblefakie mentioned this pull request May 27, 2024

Backport to branch(3) : Allow directly executing CRUD operations with transaction managers #1773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow directly executing CRUD operations with transaction managers #1755

Allow directly executing CRUD operations with transaction managers #1755

brfrn169 commented May 21, 2024

brfrn169 May 21, 2024

brfrn169 May 21, 2024

brfrn169 May 21, 2024

brfrn169 May 21, 2024

brfrn169 May 21, 2024

brfrn169 May 21, 2024

komamitsu May 22, 2024

brfrn169 May 23, 2024

komamitsu May 23, 2024

brfrn169 May 23, 2024

komamitsu May 23, 2024

brfrn169 May 24, 2024

komamitsu May 22, 2024

brfrn169 May 23, 2024

brfrn169 May 23, 2024 •

edited

Loading

komamitsu May 22, 2024

brfrn169 May 23, 2024

Torch3333 left a comment

Torch3333 May 23, 2024

brfrn169 May 24, 2024

komamitsu left a comment

feeblefakie left a comment

		public interface DistributedTransactionManager
		extends TransactionManagerCrudOperable, AutoCloseable {

		public interface TwoPhaseCommitTransactionManager
		extends TransactionManagerCrudOperable, AutoCloseable {

	LoggerFactory.getLogger(AbstractDistributedTransactionManager.class);
	LoggerFactory.getLogger(AbstractTwoPhaseCommitTransactionManager.class);

Allow directly executing CRUD operations with transaction managers #1755

Allow directly executing CRUD operations with transaction managers #1755

Conversation

brfrn169 commented May 21, 2024

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

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

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 May 23, 2024 • edited Loading

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

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

brfrn169 May 23, 2024 •

edited

Loading