sashapolo commented on code in PR #5401:
URL: https://github.com/apache/ignite-3/pull/5401#discussion_r1992172228
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientPrimaryReplicaTracker.java:
##########
@@ -439,4 +456,28 @@ public int partitions() {
return partitions;
}
}
+
+ private static final class PrimaryReplicasMapKey {
Review Comment:
Where is this class used?
##########
modules/client-handler/src/test/java/org/apache/ignite/client/handler/ClientPrimaryReplicaTrackerTest.java:
##########
@@ -52,7 +53,7 @@ class ClientPrimaryReplicaTrackerTest extends
BaseIgniteAbstractTest {
@BeforeEach
public void setUp() throws Exception {
driver = new FakePlacementDriver(PARTITIONS);
- driver.setReplicas(List.of("s1", "s2"), TABLE_ID, 1);
+ driver.setReplicas(List.of("s1", "s2"), TABLE_ID, ZONE_ID, 1);
Review Comment:
Do I understand correctly, that you've also run these tests in the
colocation branch?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientPrimaryReplicaTracker.java:
##########
@@ -75,7 +79,7 @@
* Don't block the client for too long, it is better to miss the primary
than to delay the request.
*/
public class ClientPrimaryReplicaTracker {
- private final ConcurrentHashMap<TablePartitionId, ReplicaHolder>
primaryReplicas = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<ReplicationGroupId, ReplicaHolder>
primaryReplicas = new ConcurrentHashMap<>();
Review Comment:
```suggestion
private final ConcurrentMap<ReplicationGroupId, ReplicaHolder>
primaryReplicas = new ConcurrentHashMap<>();
```
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientPrimaryReplicaTracker.java:
##########
@@ -322,11 +336,14 @@ private void onTableDrop(DropTableEventParameters
parameters) {
private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLockSafe(busyLock, () -> {
- int earliestVersion =
catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-24345 -
support zone destruction.
+ if (!enabledColocation) {
Review Comment:
Why do we not remove tables in the colocation case?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientPrimaryReplicaTracker.java:
##########
@@ -247,11 +253,26 @@ private PrimaryReplicasResult primaryReplicasNoWait(
return hasKnown ? new PrimaryReplicasResult(res, currentMaxStartTime)
: null;
}
+ private ReplicationGroupId replicationGroupId(int tableId, int partition,
HybridTimestamp timestamp) {
+ if (enabledColocation) {
+ CatalogZoneDescriptor zone = requiredZone(tableId, timestamp);
Review Comment:
You only need the table descriptor here, it already contains the zone ID.
Also, we get the zone descriptor anyway in `partitionsNoWait`, so maybe we need
to rework this code a little bit.
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ItInternalTableReadWriteScanTest.java:
##########
@@ -38,6 +40,7 @@
/**
* Tests for {@link InternalTable#scan(int, InternalTransaction)}.
*/
+@WithSystemProperty(key = IgniteSystemProperties.COLOCATION_FEATURE_FLAG,
value = "false")
Review Comment:
Didn't we agree to add TODOs in such cases?
##########
modules/client/src/test/java/org/apache/ignite/client/PartitionAwarenessTest.java:
##########
@@ -181,7 +185,7 @@ public void testNonNullTxDisablesPartitionAwareness() {
}
@ParameterizedTest
- @ValueSource(booleans = {true, false})
Review Comment:
Why did you do this?
##########
modules/client-handler/src/testFixtures/java/org/apache/ignite/client/handler/FakeCatalogService.java:
##########
@@ -53,7 +53,7 @@ public FakeCatalogService(int partitions) {
0,
0,
"table",
- 0,
+ invocation.<Integer>getArgument(0) + 10_000,
Review Comment:
please extract `invocation.getArgument(0)` into a variable. Also, why is
this change needed?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
