sanpwc commented on code in PR #2978:
URL: https://github.com/apache/ignite-3/pull/2978#discussion_r1431487158
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -438,6 +443,14 @@ public TableManager(
int cpus = Runtime.getRuntime().availableProcessors();
+ partitionOperationsExecutor = new StripedThreadPoolExecutor(
+ Math.min(cpus * 3, 25),
+ NamedThreadFactory.threadPrefix(nodeName,
"storage-operations"),
Review Comment:
Why it's "storage-operations?, it's rather a "partition-operations" one.
It's not about the naming, it's about whether we need to move only storage
related logic to the new thread or whole replicaRequest processing. I believe
that latter is better.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -3640,7 +3668,7 @@ private void failIfSchemaChangedSinceTxStart(UUID txId,
HybridTimestamp operatio
}
private CompletableFuture<Integer>
reliableCatalogVersionFor(HybridTimestamp ts) {
- return schemaSyncService.waitForMetadataCompleteness(ts)
+ return waitForMetadataCompleteness(ts)
.thenApply(unused ->
catalogService.activeCatalogVersion(ts.longValue()));
}
Review Comment:
Switch from raft to replica is missing. I'm also mentioned it in the comment
above within the context of decorated raft client.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -338,7 +345,12 @@ public PartitionReplicaListener(
cursors = new
ConcurrentSkipListMap<>(IgniteUuid.globalOrderComparator());
- schemaCompatValidator = new
SchemaCompatibilityValidator(validationSchemasSource, catalogService,
schemaSyncService);
+ schemaCompatValidator = new SchemaCompatibilityValidator(
Review Comment:
As you can see from the comments above I don't like an idea of mixing
business logic inside PartitionReplicaListener with tread switching one. I'd
say that we should propagate decorated schemaSyncService, raft client that will
switch tread on replica enter.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -468,6 +481,10 @@ public CompletableFuture<ReplicaResult>
invoke(ReplicaRequest request, String se
});
}
+ private CompletableFuture<Void> switchToRequestOperationsThread() {
+ return runAsync(NO_OP, requestOperationsExecutor);
Review Comment:
Wondering whether it's an enough efficient why to switch the thread. Any
thoughts?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -438,6 +443,14 @@ public TableManager(
int cpus = Runtime.getRuntime().availableProcessors();
+ partitionOperationsExecutor = new StripedThreadPoolExecutor(
Review Comment:
I'd rather move it to ReplicaManager. By the way, by doing this you will
eliminate the necessity of adding new parameter to PartitionReplicaListener,
instead it should be possible to change the thread somewhere near the
`replica.processRequest. It might be fine to have such parameter in
Replica.java though.
--
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]
