PakhomovAlexander commented on code in PR #4902: URL: https://github.com/apache/ignite-3/pull/4902#discussion_r1916422411
########## modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/CommonTestScheduler.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.testframework; + +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import org.apache.ignite.internal.logger.IgniteLogger; +import org.apache.ignite.internal.logger.Loggers; +import org.apache.ignite.internal.thread.NamedThreadFactory; + +/** + * Provides access to a single-thread scheduler for light-weight non-blocking operations. It doesn't need to be stopped. + */ +public class CommonTestScheduler { + private static final IgniteLogger LOG = Loggers.forClass(CommonTestScheduler.class); + + private static final ScheduledExecutorService INSTANCE = Executors.newSingleThreadScheduledExecutor( Review Comment: I far as I can see this scheduler can be used by any test in any context. The one might use this for blocking operations and make tests to be slower/flaky. I can see the javadoc that says "for light-weight non-blocking operations" but it can be missed. I would like to discuss if we really need this single-treaded scheduler in `testframework`. ########## modules/transactions/src/test/java/org/apache/ignite/internal/tx/impl/TransactionExpirationRegistryTest.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.tx.impl; + +import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import org.apache.ignite.internal.hlc.HybridTimestamp; +import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest; +import org.apache.ignite.internal.tx.InternalTransaction; +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 TransactionExpirationRegistryTest extends BaseIgniteAbstractTest { + private final TransactionExpirationRegistry registry = new TransactionExpirationRegistry(); + + @Mock + private InternalTransaction tx1; + + @Mock + private InternalTransaction tx2; + + @BeforeEach + void configureMocks() { + lenient().when(tx1.rollbackAsync()).thenReturn(nullCompletedFuture()); + lenient().when(tx2.rollbackAsync()).thenReturn(nullCompletedFuture()); + } + + @Test + void abortsTransactionsBeforeExpirationTime() { + registry.register(tx1, new HybridTimestamp(1000, 0)); Review Comment: Why do we use `HybridTimestamp` here? I think it is enough to use just millis. ########## modules/api/src/main/java/org/apache/ignite/tx/TransactionOptions.java: ########## @@ -39,10 +39,17 @@ public long timeoutMillis() { /** * Sets transaction timeout, in milliseconds. * - * @param timeoutMillis Transaction timeout, in milliseconds. + * @param timeoutMillis Transaction timeout, in milliseconds. Cannot be negative; 0 means 'use default timeout'. + * For RO transactions, the default timeout is data availability time configured via ignite.gc.lowWatermark.dataAvailabilityTime Review Comment: I think we should use another configuration property here: `transaction.timeout`. But it might be changed later. ########## modules/transactions/src/test/java/org/apache/ignite/internal/tx/impl/TransactionExpirationRegistryTest.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.tx.impl; + +import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import org.apache.ignite.internal.hlc.HybridTimestamp; +import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest; +import org.apache.ignite.internal.tx.InternalTransaction; +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 TransactionExpirationRegistryTest extends BaseIgniteAbstractTest { + private final TransactionExpirationRegistry registry = new TransactionExpirationRegistry(); + + @Mock + private InternalTransaction tx1; + + @Mock + private InternalTransaction tx2; + + @BeforeEach + void configureMocks() { + lenient().when(tx1.rollbackAsync()).thenReturn(nullCompletedFuture()); + lenient().when(tx2.rollbackAsync()).thenReturn(nullCompletedFuture()); + } + + @Test + void abortsTransactionsBeforeExpirationTime() { + registry.register(tx1, new HybridTimestamp(1000, 0)); Review Comment: I mean all time-related calculations are done on one coordinator. So, do we really need `HybridTimestamp`? ########## modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java: ########## @@ -415,17 +455,40 @@ public InternalTransaction begin( try { CompletableFuture<Void> txFuture = new CompletableFuture<>(); + + var transaction = new ReadOnlyTransactionImpl(this, timestampTracker, txId, localNodeId, implicit, readTimestamp, txFuture); + + // Implicit transactions are finished as soon as their operation/query is finished, they cannot be abandoned, so there is + // no need to register them. Review Comment: But what if the operation is tooo long? Can we do something with that? -- 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]
