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.

Sign up for GitHub

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

Enable setting of transaction expiration handler in active transaction management #1395

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Enable setting of transaction expiration handler in active transaction management #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a066db
Enable setting of transaction expiration handler in active transactio…
brfrn169 Dec 20, 2023
e33e5b1
Merge branch 'master' into enable-setting-of-transaction-expiration-h…
brfrn169 Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions ...main/java/com/scalar/db/common/ActiveTransactionManagedDistributedTransactionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.BiConsumer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -32,6 +34,17 @@ public abstract class ActiveTransactionManagedDistributedTransactionManager

private final ActiveExpiringMap<String, ActiveTransaction> activeTransactions;

private final AtomicReference<BiConsumer<String, DistributedTransaction>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question.
I think we also have this expiration handler in ScalarDB Cluster, but I'm wondering if we need to have it in both sides: ScalarDB core and ScalarDB Cluster.
If the core implements it, the one in ScalarDB Cluster is not necessary?

Copy link
Collaborator Author

@brfrn169 brfrn169 Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I can't remember the expiration handler in ScalarDB Cluster. Please give me some more information about that?

transactionExpirationHandler =
new AtomicReference<>(
(id, t) -> {
try {
t.rollback();
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] I think this rollback is always necessary and any custom expiration handler must call it. But users of setTransactionExpirationHandler() can pass a custom expiration handler that doesn't call rollback by mistake. Another option would be to add a callback like this?

private final AtomicReference<BiConsumer<String, DistributedTransaction>> transactionExpirationCallback = new AtomicReference<>((id, t) -> {});
private final BiConsumer<String, DistributedTransaction>
      transactionExpirationHandler =
              (id, t) -> {
                try {
                   transactionExpirationCallback.get().accept(id, t);
                }
                catch (Throwable t) {
                  // Warning
                }
                try {
                  t.rollback();
                } catch (Exception e) {
                  logger.warn("Rollback failed. transaction ID: {}", id, e);
                }
              };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I agree that rollback is always necessary, which is why the default handler executes rollback. This feature is advanced and is primarily intended for internal use. Additionally, I don't plan to document it. Therefore, if a user sets a custom handler that doesn't rollback the expired transaction, I believe it falls under the user's responsibility. Thanks.

} catch (Exception e) {
logger.warn("Rollback failed. transaction ID: {}", id, e);
}
});

public ActiveTransactionManagedDistributedTransactionManager(DatabaseConfig config) {
super(config);
activeTransactions =
Expand All @@ -40,14 +53,14 @@ public ActiveTransactionManagedDistributedTransactionManager(DatabaseConfig conf
TRANSACTION_EXPIRATION_INTERVAL_MILLIS,
(id, t) -> {
logger.warn("The transaction is expired. transaction ID: {}", id);
try {
t.rollback();
} catch (Exception e) {
logger.warn("Rollback failed. transaction ID: {}", id, e);
}
transactionExpirationHandler.get().accept(id, t);
});
}

public void setTransactionExpirationHandler(BiConsumer<String, DistributedTransaction> handler) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will be able to set a transaction expiration handler after this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brfrn169 Can you tell me how the custom handler would be like?

Copy link
Collaborator Author

@brfrn169 brfrn169 Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to use it for the ScalarDB Auth thing. Theoretically, we can do anything for expired transactions with this handler. For example, we can output detail logs for the expired transactions.

transactionExpirationHandler.set(handler);
}

private void add(ActiveTransaction transaction) throws TransactionException {
if (activeTransactions.putIfAbsent(transaction.getId(), transaction).isPresent()) {
transaction.rollback();
Expand Down
24 changes: 19 additions & 5 deletions ...n/java/com/scalar/db/common/ActiveTransactionManagedTwoPhaseCommitTransactionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.BiConsumer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -34,6 +36,17 @@ public abstract class ActiveTransactionManagedTwoPhaseCommitTransactionManager

private final ActiveExpiringMap<String, ActiveTransaction> activeTransactions;

private final AtomicReference<BiConsumer<String, TwoPhaseCommitTransaction>>
transactionExpirationHandler =
new AtomicReference<>(
(id, t) -> {
try {
t.rollback();
} catch (Exception e) {
logger.warn("Rollback failed. transaction ID: {}", id, e);
}
});

public ActiveTransactionManagedTwoPhaseCommitTransactionManager(DatabaseConfig config) {
super(config);
activeTransactions =
Expand All @@ -42,14 +55,15 @@ public ActiveTransactionManagedTwoPhaseCommitTransactionManager(DatabaseConfig c
TRANSACTION_EXPIRATION_INTERVAL_MILLIS,
(id, t) -> {
logger.warn("The transaction is expired. transaction ID: {}", id);
try {
t.rollback();
} catch (Exception e) {
logger.warn("Rollback failed. transaction ID: {}", id, e);
}
transactionExpirationHandler.get().accept(id, t);
});
}

public void setTransactionExpirationHandler(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto.

BiConsumer<String, TwoPhaseCommitTransaction> handler) {
transactionExpirationHandler.set(handler);
}

private void add(ActiveTransaction transaction) throws TransactionException {
if (activeTransactions.putIfAbsent(transaction.getId(), transaction).isPresent()) {
transaction.rollback();
Expand Down