denis-chudov commented on code in PR #7304:
URL: https://github.com/apache/ignite-3/pull/7304#discussion_r2727609919
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/DelayedAckException.java:
##########
@@ -31,13 +32,18 @@ public class DelayedAckException extends
IgniteInternalException {
private final UUID txId;
/**
- * Create the exception with txId and caus.
+ * Create the exception with txId, cause, and optional txManager for label
formatting.
*
* @param txId The transaction id.
* @param cause The cause.
+ * @param txManager Optional transaction manager to retrieve label for
logging.
Review Comment:
Why "optional"? It's not `@Nullable` and mandatory if `txId` is not null.
Let's make it truly optional
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1039,9 +1040,12 @@ public class IgniteImpl implements Ignite {
resourcesRegistry = new RemotelyTriggeredResourceRegistry();
- var transactionInflights = new
TransactionInflights(placementDriverMgr.placementDriver(), clockService);
+ VolatileTxStateMetaStorage txStateVolatileStorage = new
VolatileTxStateMetaStorage();
- LockManager lockMgr = new HeapLockManager(systemConfiguration);
+ TransactionInflights transactionInflights =
+ new TransactionInflights(placementDriverMgr.placementDriver(),
clockService, txStateVolatileStorage);
+
+ LockManager lockMgr = new HeapLockManager(systemConfiguration,
txStateVolatileStorage);
Review Comment:
lockMgr and txStateVolatileStorage are both started by tx manager in
`TxManagerImpl#startAsync`. Tx state storage is passed to lock manager.
But in `TxManagerImpl#startAsync` first we start lock manager, and then tx
state storage, so after the lock manager start, tx state storage is still not
started. Maybe move tx state storage start before lock manager start? It
shouldn't break anything as I see...
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/PossibleDeadlockOnLockAcquireException.java:
##########
@@ -32,18 +34,25 @@ public class PossibleDeadlockOnLockAcquireException extends
LockException {
* @param currentLockHolderTxId UUID of a transaction that currently holds
the lock.
* @param attemptedLockModeToAcquireWith {@link LockMode} that was tried
to acquire the lock with but failed the attempt.
* @param currentlyAcquiredLockMode {@link LockMode} of the lock that is
already acquired with.
+ * @param abandonedLock flag which shows the status of the lock. If the
state is uncertain, it's treated as abandoned.
+ * @param txStateMetaStorage txState required to properly log tx labels.
*/
public PossibleDeadlockOnLockAcquireException(
UUID failedToAcquireLockTxId,
UUID currentLockHolderTxId,
LockMode attemptedLockModeToAcquireWith,
- LockMode currentlyAcquiredLockMode
+ LockMode currentlyAcquiredLockMode,
+ boolean abandonedLock,
+ VolatileTxStateMetaStorage txStateMetaStorage
) {
super(
ACQUIRE_LOCK_ERR,
- "Failed to acquire a lock due to a possible deadlock ["
- + "failedToAcquireLockTransactionId=" +
failedToAcquireLockTxId
- + ", currentLockHolderTransactionId=" +
currentLockHolderTxId
+ "Failed to acquire " + (abandonedLock ? "the abandoned " : "a
")
+ + "lock due to a possible deadlock ["
+ + "failedToAcquireLockTransactionId txn "
+ + formatTxInfo(failedToAcquireLockTxId,
txStateMetaStorage)
+ + ", currentLockHolderTransactionId txn"
Review Comment:
```suggestion
+ ", currentLockHolderTransactionId=txn"
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/PossibleDeadlockOnLockAcquireException.java:
##########
@@ -32,18 +34,25 @@ public class PossibleDeadlockOnLockAcquireException extends
LockException {
* @param currentLockHolderTxId UUID of a transaction that currently holds
the lock.
* @param attemptedLockModeToAcquireWith {@link LockMode} that was tried
to acquire the lock with but failed the attempt.
* @param currentlyAcquiredLockMode {@link LockMode} of the lock that is
already acquired with.
+ * @param abandonedLock flag which shows the status of the lock. If the
state is uncertain, it's treated as abandoned.
+ * @param txStateMetaStorage txState required to properly log tx labels.
*/
public PossibleDeadlockOnLockAcquireException(
UUID failedToAcquireLockTxId,
UUID currentLockHolderTxId,
LockMode attemptedLockModeToAcquireWith,
- LockMode currentlyAcquiredLockMode
+ LockMode currentlyAcquiredLockMode,
+ boolean abandonedLock,
+ VolatileTxStateMetaStorage txStateMetaStorage
) {
super(
ACQUIRE_LOCK_ERR,
- "Failed to acquire a lock due to a possible deadlock ["
- + "failedToAcquireLockTransactionId=" +
failedToAcquireLockTxId
- + ", currentLockHolderTransactionId=" +
currentLockHolderTxId
+ "Failed to acquire " + (abandonedLock ? "the abandoned " : "a
")
+ + "lock due to a possible deadlock ["
+ + "failedToAcquireLockTransactionId txn "
Review Comment:
```suggestion
+ "failedToAcquireLockTransactionId=txn "
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandler.java:
##########
@@ -553,7 +581,13 @@ private void performAddWriteWithCleanup(
UUID wiTxId = result.currentWriteIntentTxId();
if (lastCommitTs == null) {
- throw new TxIdMismatchException(wiTxId, txId);
+
+ String formattedMessage = txManager != null ? format(
+ "Mismatched transaction id [ expectedTxId={},
conflictingTxId= {}]",
Review Comment:
```suggestion
"Mismatched transaction id [expectedTxId={},
conflictingTxId={}]",
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -866,15 +858,24 @@ private boolean isWaiterReadyToNotify(WaiterImpl waiter,
boolean skipFail) {
if (currentlyAcquiredLockMode != null &&
!currentlyAcquiredLockMode.isCompatible(intendedLockMode)) {
if (conflictFound(waiter.txId())) {
- waiter.fail(abandonedLockException(waiter.txId,
tmp.txId));
+ waiter.fail(new PossibleDeadlockOnLockAcquireException(
Review Comment:
Let's add comment that abandoned lock exception is thrown here
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -892,15 +893,23 @@ private boolean isWaiterReadyToNotify(WaiterImpl waiter,
boolean skipFail) {
if (skipFail) {
return false;
} else if (conflictFound(waiter.txId())) {
- waiter.fail(abandonedLockException(waiter.txId,
tmp.txId));
-
+ waiter.fail(new PossibleDeadlockOnLockAcquireException(
+ waiter.txId,
+ tmp.txId,
+ intendedLockMode,
+ currentlyAcquiredLockMode,
+ true,
Review Comment:
and here
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/handlers/TxCleanupRecoveryRequestHandler.java:
##########
@@ -152,7 +153,11 @@ private CompletableFuture<?> callCleanup(TxMeta txMeta,
UUID txId) {
txMeta.commitTimestamp(),
txId
).exceptionally(throwable -> {
- LOG.warn("Failed to cleanup transaction [txId={}].", throwable,
txId);
+ LOG.warn(
+ "Failed to cleanup transaction {}.",
+ throwable,
+ TransactionLogUtils.formatTxInfo(txId, txManager)
Review Comment:
you can use method static import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
