rpuch commented on code in PR #4227: URL: https://github.com/apache/ignite-3/pull/4227#discussion_r1719909118
########## modules/table/src/main/java/org/apache/ignite/internal/table/RowIdGenerator.java: ########## @@ -0,0 +1,36 @@ +/* + * 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.table; + +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.ignite.internal.util.FastTimestamps; + +/** + * New row id allocator. + */ +public class RowIdGenerator { + /** + * Get next row id. + * + * @return Next row id. + */ + public static UUID next() { + return new UUID(FastTimestamps.coarseCurrentTimeMillis(), ThreadLocalRandom.current().nextLong()); Review Comment: Is it enough to have 64 random bits? Is using 2 random longs too slow? ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/UpsertKvBenchmark.java: ########## @@ -0,0 +1,109 @@ +/* + * 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.benchmark; + +import java.util.concurrent.TimeUnit; +import org.apache.ignite.internal.lang.IgniteSystemProperties; +import org.apache.ignite.table.KeyValueView; +import org.apache.ignite.table.Tuple; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * Benchmark for a single upsert operation via KV API with a possibility to disable updates via RAFT and to storage. + */ +@State(Scope.Benchmark) +@Fork(1) +@Threads(1) +@Warmup(iterations = 10, time = 2) +@Measurement(iterations = 20, time = 2) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class UpsertKvBenchmark extends AbstractMultiNodeBenchmark { + private final Tuple tuple = Tuple.create(); + + private int id = 0; + + private static KeyValueView<Tuple, Tuple> kvView; + + @Override + public void nodeSetUp() throws Exception { + System.setProperty(IgniteSystemProperties.SKIP_REPLICATION_IN_BENCHMARK, "true"); + System.setProperty(IgniteSystemProperties.SKIP_STORAGE_UPDATE_IN_BENCHMARK, "true"); + super.nodeSetUp(); + } + + /** + * Initializes the tuple. + */ + @Setup + public void setUp() { + kvView = clusterNode.tables().table(TABLE_NAME).keyValueView(); + for (int i = 1; i < 11; i++) { + tuple.set("field" + i, FIELD_VAL); + } + } + + @TearDown + public void tearDown() { + System.out.println("Inserted " + id + " tuples"); Review Comment: How about using a logger? ########## modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java: ########## @@ -591,96 +622,25 @@ private CompletableFuture<Void> triggerTxRecovery(UUID txId, String senderId) { } /** - * Validates that the table exists at a timestamp corresponding to the request operation. + * Return tx operation timestamp. Review Comment: BTW, don't we need the detailed breakdown which timestamp is returned for which request type? This information was in the javadoc, but now it's removed. ########## modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java: ########## @@ -1982,28 +1942,34 @@ private <T> CompletableFuture<T> resolveRowByPk( assert pkLocker != null; - return pkLocker.locksForLookupByKey(txId, pk) - .thenCompose(ignored -> { + CompletableFuture<Void> lockFut = pkLocker.locksForLookupByKey(txId, pk); - boolean cursorClosureSetUp = false; - Cursor<RowId> cursor = null; + Supplier<CompletableFuture<T>> sup = () -> { + boolean cursorClosureSetUp = false; + Cursor<RowId> cursor = null; - try { - cursor = getFromPkIndex(pk); + try { + cursor = getFromPkIndex(pk); - Cursor<RowId> finalCursor = cursor; - CompletableFuture<T> resolvingFuture = continueResolvingByPk(cursor, txId, action) - .whenComplete((res, ex) -> finalCursor.close()); + Cursor<RowId> finalCursor = cursor; + CompletableFuture<T> resolvingFuture = continueResolvingByPk(cursor, txId, action) + .whenComplete((res, ex) -> finalCursor.close()); - cursorClosureSetUp = true; + cursorClosureSetUp = true; - return resolvingFuture; - } finally { - if (!cursorClosureSetUp && cursor != null) { - cursor.close(); - } - } - }); + return resolvingFuture; + } finally { + if (!cursorClosureSetUp && cursor != null) { + cursor.close(); + } + } + }; + + if (lockFut.isDone() && !lockFut.isCompletedExceptionally()) { Review Comment: ```suggestion if (CompletableFutures.isCompletedSuccessfully(lockFut)) { ``` ########## modules/core/src/main/java/org/apache/ignite/internal/util/FastTimestamps.java: ########## @@ -23,7 +23,7 @@ public class FastTimestamps { private static volatile long coarseCurrentTimeMillis = System.currentTimeMillis(); - private static final long UPDATE_FREQUENCY_MS = 10; + private static final long UPDATE_FREQUENCY_MS = 1; Review Comment: Would it make sense to add a comment saying that this cannot be more than 1ms as hybrid clock implementation uses this class? Otherwise someone might want to change it to to a higher value. ########## modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java: ########## @@ -591,96 +622,25 @@ private CompletableFuture<Void> triggerTxRecovery(UUID txId, String senderId) { } /** - * Validates that the table exists at a timestamp corresponding to the request operation. + * Return tx operation timestamp. Review Comment: ```suggestion * Returns tx operation timestamp. ``` ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/AbstractMultiNodeBenchmark.java: ########## @@ -68,14 +68,14 @@ public class AbstractMultiNodeBenchmark { protected static IgniteImpl clusterNode; - @Param({"false", "true"}) + @Param({"false"}) Review Comment: Is this change intentional? ########## modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java: ########## @@ -101,9 +101,11 @@ public IgniteBiTuple<ClusterNode, Long> enlist( TablePartitionId tablePartitionId, IgniteBiTuple<ClusterNode, Long> nodeAndConsistencyToken ) { - checkEnlistPossibility(); - - enlistPartitionLock.readLock().lock(); + // No need to wait for lock if commit is in progress. Review Comment: How do we know commit is in progress? `finishFuture` is not checked, it seems... ########## modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java: ########## @@ -513,13 +519,38 @@ private CompletableFuture<?> processRequest(ReplicaRequest request, @Nullable Bo return processGetEstimatedSizeRequest(); } - HybridTimestamp opTsIfDirectRo = (request instanceof ReadOnlyDirectReplicaRequest) ? clockService.now() : null; + @Nullable HybridTimestamp opTs = getTxOpTimestamp(request); + @Nullable HybridTimestamp opTsIfDirectRo = (request instanceof ReadOnlyDirectReplicaRequest) ? opTs : null; + @Nullable HybridTimestamp txTs = getTxStartTimestamp(request); + if (txTs == null) { Review Comment: It seems safer to switch to `request instanceof ReadOnlyDirectReplicaRequest` condition (or at least assert it) -- 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]
