Refactor transaction decoration mechanism #2585

brfrn169 · 2025-04-08T14:12:51Z

Description

This PR refactors the transaction decoration mechanism.

Related issues and/or PRs

N/A

Changes made

Deleted DistributedTransactionDecorator, DistributedTransactionDecoratorAddable, TwoPhaseCommitTransactionDecorator, and TwoPhaseCommitTransactionDecoratorAddable.
Refactored the following classes from using inheritance to using composition (decorator pattern):
- ActiveTransactionManagedDistributedTransactionManager
- ActiveTransactionManagedTwoPhaseCommitTransactionManager
- StateManagedDistributedTransactionManager
- StateManagedTwoPhaseCommitTransactionManager

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
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

N/A

brfrn169 · 2025-04-09T03:06:31Z

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

-public abstract class ActiveTransactionManagedDistributedTransactionManager
-    extends TransactionDecorationDistributedTransactionManager
-    implements DistributedTransactionExpirationHandlerSettable {
+public class ActiveTransactionManagedDistributedTransactionManager


Refactored this from using inheritance to using composition (decorator pattern).

There is now StateManagedDistributedTransactionManager that can have Status.ACTIVE. So, this class prefix "Active" seems to related to the state and a bit confusing to me. I think this class provides the capability to resume and join a transaction, so how about renaming to something like ResumableDistributedTransactionManager?

Thanks, that makes sense. However, we already have the property scalar.db.active_transaction_management.expiration_time_millis for configuring the expiration time of active transaction management:

scalardb/core/src/main/java/com/scalar/db/config/DatabaseConfig.java

Lines 51 to 52 in 0fd37fa

public static final String ACTIVE_TRANSACTION_MANAGEMENT_EXPIRATION_TIME_MILLIS =

PREFIX + "active_transaction_management.expiration_time_millis";

So I think it's better to keep the class name as is for consistency. Thanks.

brfrn169 · 2025-04-09T03:07:44Z

...main/java/com/scalar/db/common/ActiveTransactionManagedTwoPhaseCommitTransactionManager.java

-public abstract class ActiveTransactionManagedTwoPhaseCommitTransactionManager
-    extends TransactionDecorationTwoPhaseCommitTransactionManager
-    implements TwoPhaseCommitTransactionExpirationHandlerSettable {
+public class ActiveTransactionManagedTwoPhaseCommitTransactionManager


Ditto. Refactored this from using inheritance to using composition (decorator pattern).

brfrn169 · 2025-04-09T03:08:03Z

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

-public abstract class TransactionDecorationDistributedTransactionManager
-    extends AbstractDistributedTransactionManager
-    implements DistributedTransactionDecoratorAddable {
+public class StateManagedDistributedTransactionManager


Ditto. Refactored this from using inheritance to using composition (decorator pattern).

brfrn169 · 2025-04-09T03:08:10Z

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

-public abstract class TransactionDecorationTwoPhaseCommitTransactionManager
-    extends AbstractTwoPhaseCommitTransactionManager
-    implements TwoPhaseCommitTransactionDecoratorAddable {
+public class StateManagedTwoPhaseCommitTransactionManager


Ditto. Refactored this from using inheritance to using composition (decorator pattern).

brfrn169 · 2025-04-09T03:10:02Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitProvider.java

  @Override
  public DistributedTransactionManager createDistributedTransactionManager(DatabaseConfig config) {
-    return new ConsensusCommitManager(config);
+    return new ActiveTransactionManagedDistributedTransactionManager(


Wrapped the Consensus Commit transaction manager instance using decorator pattern.

brfrn169 · 2025-04-09T03:10:08Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitProvider.java

  public TwoPhaseCommitTransactionManager createTwoPhaseCommitTransactionManager(
      DatabaseConfig config) {
-    return new TwoPhaseConsensusCommitManager(config);
+    return new ActiveTransactionManagedTwoPhaseCommitTransactionManager(


brfrn169 · 2025-04-09T03:10:38Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionProvider.java

  @Override
  public DistributedTransactionManager createDistributedTransactionManager(DatabaseConfig config) {
-    return new JdbcTransactionManager(config);
+    return new ActiveTransactionManagedDistributedTransactionManager(


Wrapped the JDBC transaction manager instance using decorator pattern.

komamitsu · 2025-04-09T06:24:07Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java

-      throw new CrudException(e.getMessage(), e, e.getTransactionId().orElse(null));
-    }
+    String txId = UUID.randomUUID().toString();
+    DistributedTransaction transaction = begin(txId);


Suggested change

DistributedTransaction transaction = begin(txId);

DistributedTransaction transaction = begin();

[minor] A bit simpler?

Fixed in 41b597a. Thanks!

komamitsu · 2025-04-09T06:26:51Z

.../src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java

-      throw new CrudException(e.getMessage(), e, e.getTransactionId().orElse(null));
-    }
+    String txId = UUID.randomUUID().toString();
+    TwoPhaseCommitTransaction transaction = begin(txId);


Same as above

Fixed in 41b597a. Thanks!

komamitsu · 2025-04-09T06:32:37Z

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

-public abstract class ActiveTransactionManagedDistributedTransactionManager
-    extends TransactionDecorationDistributedTransactionManager
-    implements DistributedTransactionExpirationHandlerSettable {
+public class ActiveTransactionManagedDistributedTransactionManager


There is now StateManagedDistributedTransactionManager that can have Status.ACTIVE. So, this class prefix "Active" seems to related to the state and a bit confusing to me. I think this class provides the capability to resume and join a transaction, so how about renaming to something like ResumableDistributedTransactionManager?

komamitsu

LGTM! 👍

Torch3333

LGTM, thank you!

feeblefakie

LGTM! Thank you!

Can you elaborate the reason for the refactoring?
The old way got too complicated?
I'm just wondering.

brfrn169 · 2025-04-15T04:56:13Z

@feeblefakie

Can you elaborate the reason for the refactoring?
The old way got too complicated?

Yes. In this case, using inheritance makes the classes stateful, which complicates things. Using composition (the decorator pattern) makes future refactoring easier. Thanks!

brfrn169 added the improvement label Apr 8, 2025

brfrn169 self-assigned this Apr 8, 2025

Refactor transaction decoration mechanism

0fd37fa

brfrn169 force-pushed the refactor-transaction-decoration-mechanism branch from 0ca09e8 to 0fd37fa Compare April 9, 2025 01:56

brfrn169 commented Apr 9, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie and komamitsu April 9, 2025 03:13

komamitsu reviewed Apr 9, 2025

View reviewed changes

Fix based on feedback

41b597a

brfrn169 requested a review from komamitsu April 9, 2025 07:22

komamitsu approved these changes Apr 9, 2025

View reviewed changes

Fix

b102bda

Torch3333 approved these changes Apr 10, 2025

View reviewed changes

feeblefakie approved these changes Apr 15, 2025

View reviewed changes

Merge branch 'master' into refactor-transaction-decoration-mechanism

4aaa334

brfrn169 merged commit beac701 into master Apr 15, 2025
48 checks passed

brfrn169 deleted the refactor-transaction-decoration-mechanism branch April 15, 2025 06:58

feeblefakie pushed a commit that referenced this pull request Apr 15, 2025

Refactor transaction decoration mechanism (#2585)

bc9331b

feeblefakie mentioned this pull request Apr 15, 2025

Backport to branch(3) : Refactor transaction decoration mechanism #2596

Merged

	public static final String ACTIVE_TRANSACTION_MANAGEMENT_EXPIRATION_TIME_MILLIS =
	PREFIX + "active_transaction_management.expiration_time_millis";

	DistributedTransaction transaction = begin(txId);
	DistributedTransaction transaction = begin();

Refactor transaction decoration mechanism #2585

Refactor transaction decoration mechanism #2585

Uh oh!

Conversation

brfrn169 commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Apr 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented Apr 8, 2025 •

edited

Loading