IGNITE-22977 #4227
IGNITE-22977 #4227
Conversation
Co-authored-by: AlexLev-Tor <ql_20@hotmail.com>
# Conflicts: # docs/_docs/index.adoc
# Conflicts: # docs/_docs/index.adoc
…nto ignite-22977
…nto ignite-22977
| public static final String SKIP_REPLICATION_IN_BENCHMARK = "SKIP_REPLICATION_IN_BENCHMARK"; | ||
|
|
||
| /** Skip storage update in a benchmark. */ | ||
| public static final String SKIP_STORAGE_UPDATE_IN_BENCHMARK = "SKIP_STORAGE_UPDATE_IN_BENCHMARK"; |
There was a problem hiding this comment.
I want that both of these properties have "IGNITE_" prefix.
| ) { | ||
| @Override | ||
| public <R> CompletableFuture<R> run(Command cmd) { | ||
| return cmd.getClass().getSimpleName().contains("UpdateCommand") ? nullCompletedFuture() : super.run(cmd); |
There was a problem hiding this comment.
I worry that if the class name changes, the code will become useless.
There was a problem hiding this comment.
This is not possible to fix due to dependency issues. UpdateCommand is not available in raft module.
| executor, | ||
| commandsMarshaller | ||
| ); | ||
| boolean inBenchmark = IgniteSystemProperties.getBoolean(IgniteSystemProperties.SKIP_REPLICATION_IN_BENCHMARK, false); |
| @@ -2795,26 +2761,28 @@ private CompletableFuture<CompletableFuture<?>> applyUpdateCommand( | |||
| return resultFuture.thenApply(res -> { | |||
| UpdateCommandResult updateCommandResult = (UpdateCommandResult) res; | |||
|
|
|||
| if (full && !updateCommandResult.isPrimaryReplicaMatch()) { | |||
| if (full && updateCommandResult != null && !updateCommandResult.isPrimaryReplicaMatch()) { | |||
There was a problem hiding this comment.
I think this is done to handle a test result (SKIP_REPLICATION_IN_BENCHMARK).
Maybe it would be better to give the correct response to avoid this checking.
There was a problem hiding this comment.
This is not possible due to dependency issues. The response class is not available in raft module.
modules/core/src/main/java/org/apache/ignite/internal/util/FastTimestamps.java
Show resolved
Hide resolved
...rc/integrationTest/java/org/apache/ignite/internal/benchmark/AbstractMultiNodeBenchmark.java
Outdated
Show resolved
Hide resolved
.../runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/UpsertKvBenchmark.java
Show resolved
Hide resolved
| * @return Next row id. | ||
| */ | ||
| public static UUID next() { | ||
| return new UUID(FastTimestamps.coarseCurrentTimeMillis(), ThreadLocalRandom.current().nextLong()); |
There was a problem hiding this comment.
Is it enough to have 64 random bits? Is using 2 random longs too slow?
There was a problem hiding this comment.
My intention to use a timestamp is to reduce duplicate value probability due to periodical nature of a generator.
Both variants seem equal from perf point of view.
I'm ok with both, but prefer to leave as is if you have no arguments against.
There was a problem hiding this comment.
It seems that 64 bits per millisecond is ok
.../transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java
Show resolved
Hide resolved
| @Nullable HybridTimestamp opTs = getTxOpTimestamp(request); | ||
| @Nullable HybridTimestamp opTsIfDirectRo = (request instanceof ReadOnlyDirectReplicaRequest) ? opTs : null; | ||
| @Nullable HybridTimestamp txTs = getTxStartTimestamp(request); | ||
| if (txTs == null) { |
There was a problem hiding this comment.
It seems safer to switch to request instanceof ReadOnlyDirectReplicaRequest condition (or at least assert it)
There was a problem hiding this comment.
There is no need in such check or assertion.
txTs can be assigned to non null value only if request instanceof ReadOnlyDirectReplicaRequest
There was a problem hiding this comment.
In the current code this is true, but if we change the code, we can inadvertently break this implicit invariant. An assertion would help to make it explicit.
There was a problem hiding this comment.
Don't think this code will change in the future.
...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| if (lockFut.isDone() && !lockFut.isCompletedExceptionally()) { |
There was a problem hiding this comment.
| if (lockFut.isDone() && !lockFut.isCompletedExceptionally()) { | |
| if (CompletableFutures.isCompletedSuccessfully(lockFut)) { |
...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java
Outdated
Show resolved
Hide resolved
...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java
Show resolved
Hide resolved
https://issues.apache.org/jira/browse/IGNITE-22977
This patch adds following optimizations:
Performance gain is about 17% according to added UpsertKvBenchmark with current master #9672234bf81b35ee367138238d5585293d910fcc
Other improvements will be done in separate tickets (like IGNITE-20869)