denis-chudov commented on code in PR #7517: URL: https://github.com/apache/ignite-3/pull/7517#discussion_r2763848544
########## modules/partition-replicator/src/test/java/org/apache/ignite/internal/partition/replicator/handlers/TxFinishReplicaRequestHandlerTest.java: ########## @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.partition.replicator.handlers; + +import static java.util.concurrent.CompletableFuture.completedFuture; +import static org.apache.ignite.internal.replicator.message.ReplicaMessageUtils.toZonePartitionIdMessage; +import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrow; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import org.apache.ignite.internal.catalog.CatalogService; +import org.apache.ignite.internal.hlc.ClockService; +import org.apache.ignite.internal.hlc.HybridTimestamp; +import org.apache.ignite.internal.partition.replicator.schema.ValidationSchemasSource; +import org.apache.ignite.internal.raft.service.RaftCommandRunner; +import org.apache.ignite.internal.replicator.ZonePartitionId; +import org.apache.ignite.internal.replicator.message.ReplicaMessagesFactory; +import org.apache.ignite.internal.schema.SchemaSyncService; +import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest; +import org.apache.ignite.internal.tx.MismatchingTransactionOutcomeInternalException; +import org.apache.ignite.internal.tx.TransactionResult; +import org.apache.ignite.internal.tx.TxManager; +import org.apache.ignite.internal.tx.TxState; +import org.apache.ignite.internal.tx.message.PartitionEnlistmentMessage; +import org.apache.ignite.internal.tx.message.TxFinishReplicaRequest; +import org.apache.ignite.internal.tx.message.TxMessagesFactory; +import org.apache.ignite.internal.tx.storage.state.TxStatePartitionStorage; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class TxFinishReplicaRequestHandlerTest extends BaseIgniteAbstractTest { + private static final long ANY_ENLISTMENT_CONSISTENCY_TOKEN = 1L; + + private final TxMessagesFactory txMessagesFactory = new TxMessagesFactory(); + private final ReplicaMessagesFactory replicaMessagesFactory = new ReplicaMessagesFactory(); + + private final ZonePartitionId replicationGroupId = new ZonePartitionId(1, 1); + + @Mock + private TxStatePartitionStorage txStatePartitionStorage; + + @Mock + private ClockService clockService; + + @Mock + private TxManager txManager; + + @Mock + private ValidationSchemasSource validationSchemasSource; + + @Mock + private SchemaSyncService schemaSyncService; + + @Mock + private CatalogService catalogService; + + @Mock + private RaftCommandRunner raftCommandRunner; + + private TxFinishReplicaRequestHandler handler; + + @BeforeEach + void setUp() { + handler = new TxFinishReplicaRequestHandler( + txStatePartitionStorage, + clockService, + txManager, + validationSchemasSource, + schemaSyncService, + catalogService, + raftCommandRunner, + replicationGroupId + ); + } + + @Test + void finishReturnsMismatchingOutcomeAfterRaftWhenCommandNotApplied() { Review Comment: Could you please add one more test, when commit is written to storage and we want to abort the transaction? ########## modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/handlers/FinishTxCommandHandler.java: ########## @@ -98,18 +97,24 @@ protected CommandResult handleInternally( commandTerm ); - // Assume that we handle the finish command only on the commit partition. - txFinishMarker.markFinished(txId, command.commit(), command.commitTimestamp(), this.replicationGroupId); - LOG.debug("Finish the transaction txId = {}, state = {}, txStateChangeRes = {}", txId, txMetaToSet, txStateChangeRes); - if (!txStateChangeRes) { - assert txMetaBeforeCas != null : "txMetaBeforeCase is null, but CAS has failed for " + txId; + if (txStateChangeRes) { + // Assume that we handle the finish command only on the commit partition. + txFinishMarker.markFinished(txId, command.commit(), command.commitTimestamp(), this.replicationGroupId); + + TransactionResult result = new TransactionResult(stateToSet, command.commitTimestamp()); - onTxStateStorageCasFail(txId, txMetaBeforeCas, txMetaToSet); + return new CommandResult(result, true); } - return new CommandResult(new TransactionResult(stateToSet, command.commitTimestamp()), true); + assert txMetaBeforeCas != null : "txMetaBeforeCas is null, but CAS has failed for " + txId; Review Comment: Let's not use `assert`. I know there are many asserts on code base but we should reduce their number, not increase it I suggest something else that explicitly produces exception/error ########## modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/handlers/FinishTxCommandHandler.java: ########## @@ -122,24 +127,14 @@ private static List<EnlistedPartitionGroup> fromPartitionMessages(List<EnlistedP return list; } - private static void onTxStateStorageCasFail(UUID txId, TxMeta txMetaBeforeCas, TxMeta txMetaToSet) { - String errorMsg = format("Failed to update tx state in the storage, transaction txId = {} because of inconsistent state," + private static void logTxStateStorageCasFail(UUID txId, TxMeta txMetaBeforeCas, TxMeta txMetaToSet) { + String errorMsg = format("Finish command skipped, transaction txId = {} because of inconsistent state," Review Comment: ```suggestion String errorMsg = format("Finish command skipped, transaction txId = {} because transaction state is already set," ``` ########## modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/handlers/FinishTxCommandHandler.java: ########## @@ -122,24 +127,14 @@ private static List<EnlistedPartitionGroup> fromPartitionMessages(List<EnlistedP return list; } - private static void onTxStateStorageCasFail(UUID txId, TxMeta txMetaBeforeCas, TxMeta txMetaToSet) { - String errorMsg = format("Failed to update tx state in the storage, transaction txId = {} because of inconsistent state," + private static void logTxStateStorageCasFail(UUID txId, TxMeta txMetaBeforeCas, TxMeta txMetaToSet) { + String errorMsg = format("Finish command skipped, transaction txId = {} because of inconsistent state," + " expected state = {}, state to set = {}", Review Comment: ```suggestion + " existing state = {}, state to set = {}", ``` -- 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]
