tkalkirill commented on code in PR #4725:
URL: https://github.com/apache/ignite-3/pull/4725#discussion_r1843382741
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvTableStorage.java:
##########
@@ -243,14 +244,22 @@ public CompletableFuture<Void>
abortRebalancePartition(int partitionId) {
}
@Override
- public CompletableFuture<Void> finishRebalancePartition(
- int partitionId,
- long lastAppliedIndex,
- long lastAppliedTerm,
- byte[] groupConfig
- ) {
+ public CompletableFuture<Void> finishRebalancePartition(int partitionId,
MvPartitionMeta partitionMeta) {
return mvPartitionStorages.finishRebalance(partitionId,
mvPartitionStorage -> {
- mvPartitionStorage.finishRebalance(lastAppliedIndex,
lastAppliedTerm, groupConfig);
+ mvPartitionStorage.finishRebalance(
+ partitionMeta.lastAppliedIndex(),
+ partitionMeta.lastAppliedTerm(),
+ partitionMeta.groupConfig()
+ );
+
+ if (partitionMeta.primaryReplicaNodeId() != null) {
+ assert partitionMeta.primaryReplicaNodeId() != null;
+ mvPartitionStorage.updateLease(
Review Comment:
```suggestion
assert partitionMeta.primaryReplicaNodeId() != null;
mvPartitionStorage.updateLease(
```
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvTableStorage.java:
##########
@@ -243,14 +244,22 @@ public CompletableFuture<Void>
abortRebalancePartition(int partitionId) {
}
@Override
- public CompletableFuture<Void> finishRebalancePartition(
- int partitionId,
- long lastAppliedIndex,
- long lastAppliedTerm,
- byte[] groupConfig
- ) {
+ public CompletableFuture<Void> finishRebalancePartition(int partitionId,
MvPartitionMeta partitionMeta) {
return mvPartitionStorages.finishRebalance(partitionId,
mvPartitionStorage -> {
- mvPartitionStorage.finishRebalance(lastAppliedIndex,
lastAppliedTerm, groupConfig);
+ mvPartitionStorage.finishRebalance(
Review Comment:
I would remake it to the `MvPartitionMeta`, at your discretion.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java:
##########
@@ -135,6 +135,27 @@ public interface PartitionAccess {
*/
long maxLastAppliedTerm();
+ /**
+ * Returns the start time of the known lease for this replication group.
+ *
+ * @return Lease start time.
+ */
Review Comment:
```suggestion
/** Returns the start time of the known lease for this replication
group. */
```
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -1117,32 +1118,34 @@ public void updateLease(
return null;
}
- AbstractWriteBatch writeBatch = requireWriteBatch();
+ saveLease(requireWriteBatch(), leaseStartTime,
primaryReplicaNodeId, primaryReplicaNodeName);
- try {
- ByteArrayOutputStream outputStream = new
ByteArrayOutputStream();
- outputStream.write(longToBytes(leaseStartTime));
+ return null;
+ });
+ }
- byte[] primaryReplicaNodeIdBytes =
uuidToBytes(primaryReplicaNodeId);
- outputStream.write(primaryReplicaNodeIdBytes);
+ private void saveLease(AbstractWriteBatch writeBatch, long leaseStartTime,
UUID primaryReplicaNodeId, String primaryReplicaNodeName) {
+ try {
+ ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
Review Comment:
Maybe make the meta serializable and write the serialization logic to it?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java:
##########
@@ -135,6 +135,27 @@ public interface PartitionAccess {
*/
long maxLastAppliedTerm();
+ /**
+ * Returns the start time of the known lease for this replication group.
+ *
+ * @return Lease start time.
+ */
+ long leaseStartTime();
+
+ /**
+ * Return the node id of the known lease for this replication group.
Review Comment:
```suggestion
* Return the node ID of the known lease for this replication group.
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/raftsnapshot/ItTableRaftSnapshotsTest.java:
##########
@@ -101,6 +106,7 @@
import org.apache.ignite.table.Table;
import org.apache.ignite.table.Tuple;
import org.apache.ignite.tx.Transaction;
+import org.jetbrains.annotations.NotNull;
Review Comment:
```suggestion
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/network/raft/PartitionSnapshotMeta.java:
##########
@@ -32,4 +33,13 @@ public interface PartitionSnapshotMeta extends SnapshotMeta {
/** Row ID for which the index needs to be built per building index ID at
the time the snapshot meta was created. */
@Nullable Map<Integer, UUID> nextRowIdToBuildByIndexId();
+
+ /** Lease start time represented as {@link HybridTimestamp#longValue()}. */
+ long leaseStartTime();
Review Comment:
`HybridTimestamp` is supported for messages.
See `org.apache.ignite.internal.network.annotations.Transferable`
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/PrimitivePartitionMeta.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.engine;
+
+import java.util.UUID;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Partition meta containing values of 'primitive' types (which are the same
for all representations of partition metadata).
+ */
+public class PrimitivePartitionMeta {
+ private final long lastAppliedIndex;
+ private final long lastAppliedTerm;
+ private final long leaseStartTime;
+ private final @Nullable UUID primaryReplicaNodeId;
+ private final @Nullable String primaryReplicaNodeName;
+
+ /** Constructor. */
+ public PrimitivePartitionMeta(
+ long lastAppliedIndex,
+ long lastAppliedTerm,
+ long leaseStartTime,
+ @Nullable UUID primaryReplicaNodeId,
+ @Nullable String primaryReplicaNodeName
+ ) {
+ this.lastAppliedIndex = lastAppliedIndex;
+ this.lastAppliedTerm = lastAppliedTerm;
+ this.leaseStartTime = leaseStartTime;
+ this.primaryReplicaNodeId = primaryReplicaNodeId;
+ this.primaryReplicaNodeName = primaryReplicaNodeName;
+ }
+
+ public long lastAppliedIndex() {
+ return lastAppliedIndex;
+ }
+
+ public long lastAppliedTerm() {
+ return lastAppliedTerm;
+ }
+
+ public long leaseStartTime() {
+ return leaseStartTime;
+ }
+
+ public @Nullable UUID primaryReplicaNodeId() {
+ return primaryReplicaNodeId;
+ }
+
+ public @Nullable String primaryReplicaNodeName() {
Review Comment:
I think it's worth indicating why it might be `null`.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java:
##########
@@ -135,6 +135,27 @@ public interface PartitionAccess {
*/
long maxLastAppliedTerm();
+ /**
+ * Returns the start time of the known lease for this replication group.
+ *
+ * @return Lease start time.
+ */
+ long leaseStartTime();
Review Comment:
Maybe return `HybridTimestamp`?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/PrimitivePartitionMeta.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.engine;
+
+import java.util.UUID;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Partition meta containing values of 'primitive' types (which are the same
for all representations of partition metadata).
+ */
+public class PrimitivePartitionMeta {
+ private final long lastAppliedIndex;
+ private final long lastAppliedTerm;
+ private final long leaseStartTime;
+ private final @Nullable UUID primaryReplicaNodeId;
+ private final @Nullable String primaryReplicaNodeName;
+
+ /** Constructor. */
+ public PrimitivePartitionMeta(
+ long lastAppliedIndex,
+ long lastAppliedTerm,
+ long leaseStartTime,
+ @Nullable UUID primaryReplicaNodeId,
+ @Nullable String primaryReplicaNodeName
+ ) {
+ this.lastAppliedIndex = lastAppliedIndex;
+ this.lastAppliedTerm = lastAppliedTerm;
+ this.leaseStartTime = leaseStartTime;
+ this.primaryReplicaNodeId = primaryReplicaNodeId;
+ this.primaryReplicaNodeName = primaryReplicaNodeName;
+ }
+
+ public long lastAppliedIndex() {
+ return lastAppliedIndex;
+ }
+
+ public long lastAppliedTerm() {
+ return lastAppliedTerm;
+ }
+
+ public long leaseStartTime() {
+ return leaseStartTime;
+ }
+
+ public @Nullable UUID primaryReplicaNodeId() {
Review Comment:
I think it's worth indicating why it might be `null`.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java:
##########
@@ -487,7 +487,16 @@ private CompletableFuture<Void>
completeRebalance(@Nullable Throwable throwable)
raftGroupConfig
);
- return
partitionSnapshotStorage.partition().finishRebalance(meta.lastIncludedIndex(),
meta.lastIncludedTerm(), raftGroupConfig);
+ return partitionSnapshotStorage.partition().finishRebalance(
+ new AccessPartitionMeta(
Review Comment:
I think we can add a constructor/static method to create a new
`AccessPartitionMeta` based on `PartitionSnapshotMeta`.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java:
##########
@@ -135,6 +135,27 @@ public interface PartitionAccess {
*/
long maxLastAppliedTerm();
+ /**
+ * Returns the start time of the known lease for this replication group.
+ *
+ * @return Lease start time.
+ */
+ long leaseStartTime();
+
+ /**
+ * Return the node id of the known lease for this replication group.
+ *
+ * @return Primary replica node id or null if there is no information
about lease in the storage.
Review Comment:
```suggestion
* @return Primary replica node ID or {@code null} if there is no
information about lease in the storage.
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/AccessPartitionMeta.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.table.distributed.raft.snapshot;
+
+import java.util.UUID;
+import org.apache.ignite.internal.raft.RaftGroupConfiguration;
+import org.apache.ignite.internal.storage.engine.MvPartitionMeta;
+import org.apache.ignite.internal.storage.engine.PrimitivePartitionMeta;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Partition metadata for {@link PartitionAccess}.
+ */
+public class AccessPartitionMeta extends PrimitivePartitionMeta {
Review Comment:
Maybe rename to `RaftSnapshotPartitionMeta` ? Otherwise it sounds like
access to meta now.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccess.java:
##########
@@ -135,6 +135,27 @@ public interface PartitionAccess {
*/
long maxLastAppliedTerm();
+ /**
+ * Returns the start time of the known lease for this replication group.
+ *
+ * @return Lease start time.
+ */
+ long leaseStartTime();
+
+ /**
+ * Return the node id of the known lease for this replication group.
+ *
+ * @return Primary replica node id or null if there is no information
about lease in the storage.
+ */
+ @Nullable UUID primaryReplicaNodeId();
+
+ /**
+ * Return the node name of the known lease for this replication group.
+ *
+ * @return Primary replica node name or null if there is no information
about lease in the storage.
Review Comment:
```suggestion
* @return Primary replica node name or {@code null} if there is no
information about lease in the storage.
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/raftsnapshot/ItTableRaftSnapshotsTest.java:
##########
@@ -200,12 +207,32 @@ private void testLeaderFeedsFollowerWithSnapshot(String
storageEngine) throws Ex
transferLeadershipOnSolePartitionTo(2);
assertThat(getFromNode(2, 1), is("one"));
+ assertThat(estimatedSizeFromNode(2), is(1L));
+
+ MvPartitionStorage partitionAtNode0 = mvPartitionAtNode(0);
+ MvPartitionStorage partitionAtNode2 = mvPartitionAtNode(2);
+
+ assertThat(partitionAtNode2.leaseStartTime(), not(0L));
+ assertEquals(partitionAtNode0.primaryReplicaNodeId(),
partitionAtNode2.primaryReplicaNodeId());
+ assertEquals(partitionAtNode0.primaryReplicaNodeName(),
partitionAtNode2.primaryReplicaNodeName());
}
private @Nullable String getFromNode(int clusterNode, int key) {
return tableViewAt(clusterNode).get(null, key);
}
+ private long estimatedSizeFromNode(int clusterNode) {
+ return mvPartitionAtNode(clusterNode).estimatedSize();
+ }
+
+ private @NotNull MvPartitionStorage mvPartitionAtNode(int clusterNode) {
Review Comment:
```suggestion
private MvPartitionStorage mvPartitionAtNode(int clusterNode) {
```
--
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
