denis-chudov commented on code in PR #7482:
URL: https://github.com/apache/ignite-3/pull/7482#discussion_r2911604339
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMeta.java:
##########
@@ -187,10 +281,52 @@ public TxState txState() {
return isFinishedDueToTimeout;
}
+ public boolean isFinishedDueToTimeoutOrFalse() {
+ return Boolean.TRUE.equals(isFinishedDueToTimeout);
+ }
+
public @Nullable String txLabel() {
return txLabel;
}
+ public @Nullable Throwable lastException() {
+ return lastException;
+ }
+
+ public @Nullable Integer lastExceptionErrorCode() {
+ return lastExceptionErrorCode;
+ }
+
+ private static @Nullable Throwable normalizeThrowable(@Nullable Throwable
throwable) {
+ return throwable == null ? null :
ExceptionUtils.unwrapRootCause(throwable);
+ }
+
+ private static @Nullable Integer normalizeErrorCode(
+ @Nullable Integer explicitErrorCode,
+ @Nullable Throwable throwable,
+ @Nullable Boolean isFinishedDueToTimeout
+ ) {
+ if (Boolean.TRUE.equals(isFinishedDueToTimeout)) {
+ return null;
+ }
+
+ if (explicitErrorCode != null) {
+ return explicitErrorCode;
+ }
+
+ Throwable normalized = normalizeThrowable(throwable);
Review Comment:
Probably may be moved to exception utils
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxStateMetaMessage.java:
##########
@@ -41,6 +41,8 @@ public interface TxStateMetaMessage extends
TransactionMetaMessage {
@Nullable Boolean isFinishedDueToTimeout();
+ @Nullable Integer lastExceptionErrorCode();
Review Comment:
Let's rename it to just `exceptionErrorCode`, I'm still not sure that we
should prefer last exception over first, let's not fix it in the network
protocol
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMeta.java:
##########
@@ -111,6 +120,89 @@ public TxStateMeta(
@Nullable Long cleanupCompletionTimestamp,
@Nullable Boolean isFinishedDueToTimeout,
@Nullable String txLabel
+ ) {
+ this(
+ txState,
+ txCoordinatorId,
+ commitPartitionId,
+ commitTimestamp,
+ tx,
+ initialVacuumObservationTimestamp,
+ cleanupCompletionTimestamp,
+ isFinishedDueToTimeout,
+ txLabel,
+ null
+ );
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param txState Transaction state.
+ * @param txCoordinatorId Transaction coordinator id.
+ * @param commitPartitionId Commit partition replication group id.
+ * @param commitTimestamp Commit timestamp.
+ * @param tx Transaction object. This parameter is not {@code null} only
for transaction coordinator.
+ * @param initialVacuumObservationTimestamp Initial vacuum observation
timestamp.
+ * @param cleanupCompletionTimestamp Cleanup completion timestamp.
+ * @param isFinishedDueToTimeout {@code true} if the transaction is
finished due to timeout.
+ * @param txLabel Transaction label.
+ * @param lastException The last exception occurred in tx.
+ */
+ public TxStateMeta(
Review Comment:
Please remove this constructor, it has only one usage in another constructor.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMeta.java:
##########
@@ -206,6 +342,7 @@ public TxStateMetaMessage toTransactionMetaMessage(
.initialVacuumObservationTimestamp(initialVacuumObservationTimestamp)
.cleanupCompletionTimestamp(cleanupCompletionTimestamp)
.isFinishedDueToTimeout(isFinishedDueToTimeout)
+ .lastExceptionErrorCode(lastExceptionErrorCode)
Review Comment:
Please also add it to `TxStateMetaAbandoned#toTransactionMetaMessage`
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -46,6 +48,8 @@
* The class represents volatile transaction state storage that stores a
transaction state meta until the node stops.
*/
public class VolatileTxStateMetaStorage {
+ private static final IgniteLogger LOG =
Loggers.forClass(VolatileTxStateMetaStorage.class);
Review Comment:
`IgniteThrottledLogger` should be here to avoid log flood
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -115,6 +119,53 @@ public void initialize(InternalTransaction tx, @Nullable
String txLabel) {
});
}
+ /**
+ * Atomically updates transaction metadata (TxStateMeta) without
validating TxState transitions.
+ * Use this only for metadata-only changes that do not affect
state-derived fields
+ * (currently exception info or labels), and never to update TxState
itself.
+ *
+ * @param txId Transaction id.
+ * @param updater Transaction meta updater.
+ * @return Updated transaction state.
+ */
+ public @Nullable <T extends TxStateMeta> T enrichMeta(UUID txId,
+ Function<@Nullable TxStateMeta, TxStateMeta> updater) {
+ return (T) txStateMap.compute(txId, (k, oldMeta) -> {
+ TxStateMeta newMeta = updater.apply(oldMeta);
+
+ if (newMeta == null) {
+ if (oldMeta != null) {
+ LOG.info("Skipped removing transaction state in enrichMeta
[txId={}].", txId);
+ }
+
+ return oldMeta;
+ }
+
+ if (!isEnrichAllowed(txId, oldMeta, newMeta)) {
+ return oldMeta;
+ }
+
+ return newMeta;
+ });
+ }
+
+ private static boolean isEnrichAllowed(UUID txId, @Nullable TxStateMeta
oldMeta, TxStateMeta newMeta) {
+ if (oldMeta == null) {
+ LOG.info("Skipped creating transaction state in enrichMeta
[txId={}].", txId);
+
+ return false;
+ }
+
+ if (!Objects.equals(oldMeta.txState(), newMeta.txState())) {
Review Comment:
txState is enum so can be compared with `!=`
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -3822,6 +3862,57 @@ private CompletableFuture<?>
processOperationRequestWithTxOperationManagementLog
}
}
+ private void storeFailureInTxMeta(ReplicaRequest request, Throwable
throwable) {
+ if (!(request instanceof ReadWriteReplicaRequest)) {
+ return;
+ }
+
+ UUID txId = ((ReadWriteReplicaRequest) request).transactionId();
+ Throwable toStore = unwrapRootCause(throwable);
+
+ txManager.enrichTxMeta(txId, old -> {
+ if (old == null || old.lastException() != null) {
+ return old;
+ }
+
+ return old.mutate()
+ .lastException(toStore)
+ .build();
+ });
+ }
+
+ private @Nullable TransactionException
transactionAlreadyFinishedException(ReplicaRequest request) {
Review Comment:
The true purpose of this method is not clear from its name and it doesn't
have the javadoc, please elaborate it
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -115,6 +119,53 @@ public void initialize(InternalTransaction tx, @Nullable
String txLabel) {
});
}
+ /**
+ * Atomically updates transaction metadata (TxStateMeta) without
validating TxState transitions.
+ * Use this only for metadata-only changes that do not affect
state-derived fields
+ * (currently exception info or labels), and never to update TxState
itself.
+ *
+ * @param txId Transaction id.
+ * @param updater Transaction meta updater.
+ * @return Updated transaction state.
+ */
+ public @Nullable <T extends TxStateMeta> T enrichMeta(UUID txId,
+ Function<@Nullable TxStateMeta, TxStateMeta> updater) {
+ return (T) txStateMap.compute(txId, (k, oldMeta) -> {
+ TxStateMeta newMeta = updater.apply(oldMeta);
+
+ if (newMeta == null) {
+ if (oldMeta != null) {
+ LOG.info("Skipped removing transaction state in enrichMeta
[txId={}].", txId);
+ }
+
+ return oldMeta;
+ }
+
+ if (!isEnrichAllowed(txId, oldMeta, newMeta)) {
+ return oldMeta;
+ }
+
+ return newMeta;
+ });
+ }
+
+ private static boolean isEnrichAllowed(UUID txId, @Nullable TxStateMeta
oldMeta, TxStateMeta newMeta) {
+ if (oldMeta == null) {
+ LOG.info("Skipped creating transaction state in enrichMeta
[txId={}].", txId);
Review Comment:
Please log new meta.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -115,6 +119,53 @@ public void initialize(InternalTransaction tx, @Nullable
String txLabel) {
});
}
+ /**
+ * Atomically updates transaction metadata (TxStateMeta) without
validating TxState transitions.
Review Comment:
` without validating TxState transitions` this is not true now.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -63,6 +71,10 @@ public class ReadWriteTransactionImpl extends
IgniteAbstractTransactionImpl {
/** The future is initialized when this transaction starts committing or
rolling back and is finished together with the transaction. */
private volatile CompletableFuture<Void> finishFuture;
+ private volatile @Nullable Throwable finishCause;
+
+ private volatile int finishCode = TX_ALREADY_FINISHED_ERR;
Review Comment:
looks weird
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -3795,10 +3827,18 @@ private CompletableFuture<?>
processOperationRequestWithTxOperationManagementLog
indexBuildingProcessor.decrementRwOperationCountIfNeeded(request);
if (ex != null) {
+ storeFailureInTxMeta(request, ex);
+
if (hasCause(ex, LockException.class)) {
RequestType failedRequestType =
getRequestOperationType(request);
- sneakyThrow(new
OperationLockException(failedRequestType, (LockException) unwrapCause(ex)));
+ TransactionException alreadyFinished =
transactionAlreadyFinishedException(request);
Review Comment:
why is it needed only in `if (hasCause(ex, LockException.class))` branch? If
this is correct, please add the comment with clarification.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMeta.java:
##########
@@ -187,10 +281,52 @@ public TxState txState() {
return isFinishedDueToTimeout;
}
+ public boolean isFinishedDueToTimeoutOrFalse() {
+ return Boolean.TRUE.equals(isFinishedDueToTimeout);
+ }
+
public @Nullable String txLabel() {
return txLabel;
}
+ public @Nullable Throwable lastException() {
+ return lastException;
+ }
+
+ public @Nullable Integer lastExceptionErrorCode() {
+ return lastExceptionErrorCode;
+ }
+
+ private static @Nullable Throwable normalizeThrowable(@Nullable Throwable
throwable) {
+ return throwable == null ? null :
ExceptionUtils.unwrapRootCause(throwable);
+ }
+
+ private static @Nullable Integer normalizeErrorCode(
+ @Nullable Integer explicitErrorCode,
+ @Nullable Throwable throwable,
+ @Nullable Boolean isFinishedDueToTimeout
+ ) {
+ if (Boolean.TRUE.equals(isFinishedDueToTimeout)) {
Review Comment:
I think, should be `TX_ALREADY_FINISHED_WITH_TIMEOUT_ERR`
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMetaFinishing.java:
##########
@@ -50,7 +74,55 @@ public TxStateMetaFinishing(
@Nullable Boolean isFinishingDueToTimeout,
@Nullable String txLabel
) {
- super(TxState.FINISHING, txCoordinatorId, commitPartitionId, null,
null, null, null, isFinishingDueToTimeout, txLabel);
+ this(txCoordinatorId, commitPartitionId, isFinishingDueToTimeout,
txLabel, null);
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param txCoordinatorId Transaction coordinator id.
+ * @param commitPartitionId Commit partition id.
+ * @param isFinishingDueToTimeout {@code true} if transaction is finishing
due to timeout, {@code false} otherwise.
+ * @param txLabel Transaction label.
+ * @param finishReason Exception which caused tx abortion.
+ */
+ public TxStateMetaFinishing(
+ @Nullable UUID txCoordinatorId,
+ @Nullable ZonePartitionId commitPartitionId,
+ @Nullable Boolean isFinishingDueToTimeout,
+ @Nullable String txLabel,
+ @Nullable Throwable finishReason
+ ) {
+ this(
+ txCoordinatorId,
+ commitPartitionId,
+ isFinishingDueToTimeout,
+ txLabel,
+ finishReason,
+ null
+ );
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param txCoordinatorId Transaction coordinator id.
+ * @param commitPartitionId Commit partition id.
+ * @param isFinishingDueToTimeout {@code true} if transaction is finishing
due to timeout, {@code false} otherwise.
+ * @param txLabel Transaction label.
+ * @param finishReason Exception which caused tx abortion.
+ * @param lastExceptionErrorCode Error code of the last exception.
+ */
+ public TxStateMetaFinishing(
+ @Nullable UUID txCoordinatorId,
+ @Nullable ZonePartitionId commitPartitionId,
+ @Nullable Boolean isFinishingDueToTimeout,
+ @Nullable String txLabel,
+ @Nullable Throwable finishReason,
+ @Nullable Integer lastExceptionErrorCode
+ ) {
+ super(TxState.FINISHING, txCoordinatorId, commitPartitionId, null,
null,
+ null, null, isFinishingDueToTimeout, txLabel, finishReason,
lastExceptionErrorCode);
Review Comment:
Please don't pile up the constructors here as well, one of them is even
never used
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMetaAbandoned.java:
##########
@@ -52,7 +52,48 @@ public TxStateMetaAbandoned(
@Nullable InternalTransaction tx,
@Nullable String txLabel
) {
- super(ABANDONED, txCoordinatorId, commitPartitionId, null, tx, null,
null, null, txLabel);
+ this(txCoordinatorId, commitPartitionId, tx, txLabel, null);
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param txCoordinatorId Transaction coordinator id.
+ * @param commitPartitionId Commit partition replication group id.
+ * @param tx Transaction object. This parameter is not {@code null} only
for transaction coordinator.
+ * @param txLabel Transaction label.
+ * @param exceptionInfo Exception info for exceptional abort.
+ */
+ public TxStateMetaAbandoned(
+ @Nullable UUID txCoordinatorId,
+ @Nullable ZonePartitionId commitPartitionId,
+ @Nullable InternalTransaction tx,
+ @Nullable String txLabel,
+ @Nullable Throwable exceptionInfo
+ ) {
+ this(txCoordinatorId, commitPartitionId, tx, txLabel, exceptionInfo,
null);
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param txCoordinatorId Transaction coordinator id.
+ * @param commitPartitionId Commit partition replication group id.
+ * @param tx Transaction object. This parameter is not {@code null} only
for transaction coordinator.
+ * @param txLabel Transaction label.
+ * @param exceptionInfo Exception info for exceptional abort.
+ * @param lastExceptionErrorCode Error code of the last exception.
+ */
+ public TxStateMetaAbandoned(
+ @Nullable UUID txCoordinatorId,
+ @Nullable ZonePartitionId commitPartitionId,
+ @Nullable InternalTransaction tx,
+ @Nullable String txLabel,
+ @Nullable Throwable exceptionInfo,
+ @Nullable Integer lastExceptionErrorCode
+ ) {
+ super(ABANDONED, txCoordinatorId, commitPartitionId, null, tx, null,
null, null, txLabel, exceptionInfo,
+ lastExceptionErrorCode);
Review Comment:
Please don't pile up the constructors, each of them is used in just one or
two places, this doesn't make sense
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxStateMeta.java:
##########
@@ -111,6 +120,89 @@ public TxStateMeta(
@Nullable Long cleanupCompletionTimestamp,
Review Comment:
The constructor at line 113 is used only in some unit tests, probably can be
unified with shortened one at line 89
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -115,6 +119,53 @@ public void initialize(InternalTransaction tx, @Nullable
String txLabel) {
});
}
+ /**
+ * Atomically updates transaction metadata (TxStateMeta) without
validating TxState transitions.
+ * Use this only for metadata-only changes that do not affect
state-derived fields
+ * (currently exception info or labels), and never to update TxState
itself.
+ *
+ * @param txId Transaction id.
+ * @param updater Transaction meta updater.
+ * @return Updated transaction state.
+ */
+ public @Nullable <T extends TxStateMeta> T enrichMeta(UUID txId,
+ Function<@Nullable TxStateMeta, TxStateMeta> updater) {
+ return (T) txStateMap.compute(txId, (k, oldMeta) -> {
+ TxStateMeta newMeta = updater.apply(oldMeta);
+
+ if (newMeta == null) {
Review Comment:
also can be packed into `isEnrichAllowed`
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxManager.java:
##########
@@ -145,6 +145,17 @@ default InternalTransaction
beginExplicitRo(HybridTimestampTracker timestampTrac
@Nullable
<T extends TxStateMeta> T updateTxMeta(UUID txId, Function<@Nullable
TxStateMeta, TxStateMeta> updater);
+ /**
+ * Atomically updates transaction metadata while bypassing TxState
transition validation.
Review Comment:
` while bypassing TxState transition validation.` this is not true now
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/VolatileTxStateMetaStorage.java:
##########
@@ -115,6 +119,53 @@ public void initialize(InternalTransaction tx, @Nullable
String txLabel) {
});
}
+ /**
+ * Atomically updates transaction metadata (TxStateMeta) without
validating TxState transitions.
+ * Use this only for metadata-only changes that do not affect
state-derived fields
+ * (currently exception info or labels), and never to update TxState
itself.
+ *
+ * @param txId Transaction id.
+ * @param updater Transaction meta updater.
+ * @return Updated transaction state.
+ */
+ public @Nullable <T extends TxStateMeta> T enrichMeta(UUID txId,
+ Function<@Nullable TxStateMeta, TxStateMeta> updater) {
+ return (T) txStateMap.compute(txId, (k, oldMeta) -> {
+ TxStateMeta newMeta = updater.apply(oldMeta);
+
+ if (newMeta == null) {
+ if (oldMeta != null) {
+ LOG.info("Skipped removing transaction state in enrichMeta
[txId={}].", txId);
Review Comment:
Please log old meta.
--
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]
