IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId #3548

alievmirza · 2024-04-03T14:05:09Z

https://issues.apache.org/jira/browse/IGNITE-21911

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

…m TablePartitionId to ZonePartitionId

JAkutenshi

Mostly questions as a review

JAkutenshi · 2024-04-10T05:38:12Z

...src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogHashIndexDescriptor.java

            boolean unique,
            CatalogIndexStatus status,
            int txWaitCatalogVersion,
+            int zoneId,


Missed zoneId in jdoc like above?

JAkutenshi · 2024-04-10T05:45:32Z

modules/core/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionId.java

+     * @param zoneId Zone id.
+     * @param partId Partition id.
+     */
+    public ZonePartitionId(int zoneId, int partId, int tableId) {


May I propose to change args order to (zoneId, tableId, partId) for call consistency? Like in ChangeIndexStatusTask:L238 will look like from more global to less global ids'.

Agree. Moreover, I'd actually adjust the fields order in the same manner.

JAkutenshi · 2024-04-10T05:50:22Z

modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildController.java

+                TablePartitionId tablePartId = new TablePartitionId(tableId, zonePartitionId.partitionId());
+
+                if (tableId == indexDescriptor.tableId()) {
+                    CompletableFuture<?> startBuildIndexFuture = getMvTableStorageFuture(parameters.causalityToken(), tablePartId)


Shouldn't getMvTableStorageFuture's second argument be changed to ZonePartitionId too?

JAkutenshi · 2024-04-10T06:08:01Z

modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildController.java

@@ -244,7 +258,7 @@ private void tryScheduleBuildIndex(
     * @param replicaId Replica ID.
     */
    private void stopBuildingIndexesIfPrimaryExpired(TablePartitionId replicaId) {


The same, shouldn't be there new ZonePartitionId?

As an additional question: isn't one of subtask is to get rid of TablePartitionId?

JAkutenshi · 2024-04-10T06:22:55Z

...rc/main/java/org/apache/ignite/internal/placementdriver/leases/ReplicaAwareLeaseTracker.java

+                    ClusterNode leaseholderNode = clusterNodeResolver.getById(replicaMeta.getLeaseholderId());
+
+                    WaitReplicaStateMessage awaitReplicaReq = REPLICA_MESSAGES_FACTORY.waitReplicaStateMessage()
+                            .groupId(tablePartitionId)


The same, as input we got ZPId as groupId, but have to use TPId

JAkutenshi · 2024-04-10T06:28:18Z

modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java

                metaStorageMgr,
                indexManager,
-                placementDriverMgr.placementDriver(),
+                new ReplicaAwareLeaseTracker(placementDriverMgr.placementDriver(), replicaSvc, clusterSvc.topologyService()),


Why we should use the delegate only for index?

sanpwc · 2024-04-10T12:03:48Z

modules/core/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionId.java

+ * The class is used to identify a zone replication group id for a given partition.
+ */
+public class ZonePartitionId implements ReplicationGroupId {
+


Please remove the empty line.

sanpwc · 2024-04-10T12:05:10Z

modules/core/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionId.java

+    /** Partition id. */
+    private final int partId;
+
+    private final int tableId;


Please either add javadoc to the tableId field or remove zoneId, partId ones. From my point of view, the latter is better here.

sanpwc · 2024-04-10T12:05:53Z

modules/core/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionId.java

+    /**
+     * The constructor.
+     *
+     * @param zoneId Zone id.


tableId param is missing.

sanpwc · 2024-04-10T12:07:50Z

modules/index/build.gradle

    implementation project(':ignite-raft-api')
    implementation project(':ignite-metastorage-api')
    implementation project(':ignite-placement-driver-api')
+    implementation project(':ignite-placement-driver')


sanpwc · 2024-04-10T12:13:37Z

...ndex/src/test/java/org/apache/ignite/internal/index/ChangeIndexStatusTaskControllerTest.java


    private void setPrimaryReplica(ClusterNode clusterNode) {
-        TablePartitionId groupId = new TablePartitionId(tableId(), 0);
+        ZonePartitionId zonePartId = new ZonePartitionId(0, 0, tableId());


Here and there. Using 0 is generally not consistent. tableId internally wil retrieve id from CatalogTableDescriptor

public static CatalogTableDescriptor getTableStrict(CatalogService catalogService, String tableName, long timestamp) { CatalogTableDescriptor table = catalogService.table(tableName, timestamp); assertNotNull(table, "tableName=" + tableName + ", timestamp=" + timestamp); return table; }

Meaning that we should retrieve zoneId from the same descriptor.

sanpwc · 2024-04-10T12:15:11Z

...ndex/src/test/java/org/apache/ignite/internal/index/ChangeIndexStatusTaskControllerTest.java

+        TablePartitionId tablePartId = new TablePartitionId(tableId(), 0);

-        assertThat(placementDriver.setPrimaryReplicaMeta(0, groupId, completedFuture(replicaMeta)), willCompleteSuccessfully());
+        ReplicaMeta replicaMeta = newPrimaryReplicaMeta(clusterNode, tablePartId, HybridTimestamp.MIN_VALUE, HybridTimestamp.MAX_VALUE);


Why it's tablePartId in replicaMeta? We should have new ZonePartitionId there.

sanpwc · 2024-04-10T12:16:56Z

modules/index/src/test/java/org/apache/ignite/internal/index/IndexBuildControllerTest.java

-    private TablePartitionId replicaId(int partitionId) {
-        return new TablePartitionId(tableId(), partitionId);
+    private ZonePartitionId replicaId(int partitionId) {
+        return new ZonePartitionId(0, partitionId, tableId());


Same as above, not valid to use 0 here as zoneId, please retrieve the identifier from catalog descriptor like tableId() does.

sanpwc · 2024-04-10T12:17:54Z

modules/index/src/test/java/org/apache/ignite/internal/index/TestPlacementDriver.java

                new PrimaryReplicaEventParameters(
                        causalityToken,
-                        replicaId,
+                        new TablePartitionId(replicaId.tableId(), replicaId.partitionId()),


We should use ZonePartitionId here.

sanpwc · 2024-04-10T12:19:43Z

...ent-driver-api/src/main/java/org/apache/ignite/internal/placementdriver/PlacementDriver.java

    );

+    /**
+     * Temporary solution for awaiting {@link ReplicaMeta}. Waits for


If it's temporary please add todo with a link and mark the method as @deprecated. Please create a ticket if necessary.

sanpwc · 2024-04-10T12:22:29Z

...ent-driver-api/src/main/java/org/apache/ignite/internal/placementdriver/PlacementDriver.java

+     * @throws PrimaryReplicaAwaitException If primary replica await failed with any other reason except timeout.
+     */
+    default CompletableFuture<ReplicaMeta> awaitPrimaryReplicaForTable(
+            ZonePartitionId groupId,


Why it's ZonePartitionId? It should be ReplicationGroupId.

sanpwc · 2024-04-10T14:37:03Z

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java

+     * @return Future that contains a result.
+     */
+    private CompletableFuture<Void> processWaitReplicaStateMessage(WaitReplicaStateMessage msg) {
+        LOG.info("Received LeaseGrantedMessage for replica belonging to group=" + groupId());


It's not LeaseGrantedMessage.

sanpwc · 2024-04-10T14:39:04Z

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java

+    private CompletableFuture<Void> processWaitReplicaStateMessage(WaitReplicaStateMessage msg) {
+        LOG.info("Received LeaseGrantedMessage for replica belonging to group=" + groupId());
+
+        return waitForActualState(FastTimestamps.coarseCurrentTimeMillis() + TimeUnit.SECONDS.toMillis(msg.timeout()));


It's also required to sendPrimaryReplicaChangeToReplicationGroup after waitForActualState. Please check Replica#processLeaseGrantedMessage for more details.

alievmirza added 5 commits April 3, 2024 18:04

IGNITE-21911 WIP

9bca4dd

IGNITE-21911 WIP

c917b1d

IGNITE-21911 WIP

37053dc

IGNITE-21911 Change API usage of Placement driver in Index module fro…

c043847

…m TablePartitionId to ZonePartitionId

Merge remote-tracking branch 'apache-ignite-3/main' into ignite-21911

9d8bc93

alievmirza changed the title ~~IGNITE-21911 WIP~~ IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId Apr 9, 2024

IGNITE-21912 Javadocs

7ddb774

JAkutenshi reviewed Apr 10, 2024

View reviewed changes

sanpwc reviewed Apr 10, 2024

View reviewed changes

alievmirza closed this Apr 30, 2024

IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId #3548

IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId #3548

Uh oh!

Conversation

alievmirza commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Review Checklist

Notes

Uh oh!

JAkutenshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanpwc Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alievmirza commented Apr 3, 2024 •

edited

Loading

sanpwc Apr 10, 2024 •

edited

Loading