sanpwc commented on code in PR #5170:
URL: https://github.com/apache/ignite-3/pull/5170#discussion_r1943375705
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/message/TableAware.java:
##########
@@ -20,9 +20,10 @@
import org.apache.ignite.internal.network.NetworkMessage;
/**
- * Generic interface for all messages about concrete table.
+ * This class represents a common base interface for all message types related
to a particular table.
+ * Extending this interface means the message is propagated to the table
replica to be processed via zone replica.
Review Comment:
table replica -> table request processor. There are no table replicas in
collocation track.
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItColocationTest.java:
##########
@@ -233,6 +233,7 @@ public CompletableFuture<Void> finish(
Map<ReplicationGroupId, RaftGroupService> groupRafts = new HashMap<>();
int tblId = 1;
+ int zoneId = 2;
Review Comment:
I do understand that it's just a test. However it's very uncommon to have
zoneId > tblId. Seems that currently it's just not possible from the production
code point of view.
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/message/TableAware.java:
##########
@@ -20,9 +20,10 @@
import org.apache.ignite.internal.network.NetworkMessage;
/**
- * Generic interface for all messages about concrete table.
+ * This class represents a common base interface for all message types related
to a particular table.
Review Comment:
Basically it's not true. There are dozen of messages that relate to
particular table but doesn't extend TableAware. This class represents a common
base interface **for replica requests** related to a particular table...
I'd also use targeted instead of related. Up to you.
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -716,6 +716,55 @@ void
testStableAreWrittenAfterRestartAndConcurrentStableUpdate(TestInfo testInfo
assertTrue(waitForCondition(() ->
getNode(0).replicaManager.isReplicaStarted(partId), 10_000L));
}
+ @Test
+ public void testTableEstimatedSize(TestInfo testInfo) throws Exception {
+ startNodes(testInfo, 3);
+
+ Assignment replicaAssignment = (Assignment)
calculateAssignmentForPartition(
+ nodes.values().stream().map(n -> n.name).collect(toList()), 0,
1, 1).toArray()[0];
+
+ Node node = getNode(replicaAssignment.consistentId());
+
+
placementDriver.setPrimary(node.clusterService.topologyService().localMember());
+
+ createZone(node, "test_zone", 1, 1);
+ int zoneId = DistributionZonesTestUtil.getZoneId(node.catalogManager,
"test_zone", node.hybridClock.nowLong());
+
+ long key = 1;
+
+ {
+ createTable(node, "test_zone", "test_table");
Review Comment:
I'd say that we need a zone with several partitions and couple of tables
with different amount of data, e.g.
```
z1 -> t1 -> p1 -> 1 record
t1-> p2 -> 2 records
t2 -> p1 -> 4 records
t2 -> p2 -> 5 records
```
And thus we may assert that t1 contains 3 records, and t2 9 recods.
I do understand that such assertions will mainly check original
estimatedSize logic and node the proposed one in given request, however it
worth almost nothing to adjust the test in that way and it'll bring us a little
more confidence.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -2128,13 +2149,21 @@ public CompletableFuture<Long> estimatedSize() {
var invokeFutures = new CompletableFuture<?>[partitions];
for (int partId = 0; partId < partitions; partId++) {
- var replicaGroupId = new TablePartitionId(tableId, partId);
+ ReplicationGroupIdMessage partitionIdMessage;
Review Comment:
Could you please add colocation cleanup todo here?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -106,7 +105,7 @@ public void testZoneNotFoundOnDrop2() {
.zoneName(ZONE_NAME)
.ifExists(true)
.build();
- assertThat(commandHandler.handle(cmd), willBe(IsNull.nullValue()));
+ assertThat(commandHandler.handle(cmd), willCompleteSuccessfully());
Review Comment:
Hmm, why?
--
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]
