rpuch commented on code in PR #4049:
URL: https://github.com/apache/ignite-3/pull/4049#discussion_r1731033124
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/StoragePartitionMeta.java:
##########
@@ -61,6 +65,8 @@ public class StoragePartitionMeta extends PartitionMeta {
* @param lastAppliedTerm Last applied term value.
* @param lastReplicationProtocolGroupConfigFirstPageId ID of the first
page in a chain storing a blob representing.
Review Comment:
```suggestion
* @param lastReplicationProtocolGroupConfigFirstPageId ID of the first
page in a chain storing a blob representing last replication protocol group
config.
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/StoragePartitionMeta.java:
##########
@@ -61,6 +65,8 @@ public class StoragePartitionMeta extends PartitionMeta {
* @param lastAppliedTerm Last applied term value.
* @param lastReplicationProtocolGroupConfigFirstPageId ID of the first
page in a chain storing a blob representing.
* @param leaseStartTime Lease start time.
+ * @param primaryReplicaNodeIdFirstPageId ID of the first page in a chain
storing a blob representing of a primary replica node id.
+ * @param primaryReplicaNodeNameFirstPageId ID of the first page in a
chain storing a blob representing of a primary replica node name.
Review Comment:
```suggestion
* @param primaryReplicaNodeNameFirstPageId ID of the first page in a
chain storing a blob representing a primary replica node name.
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/StoragePartitionMetaIo.java:
##########
@@ -48,8 +48,13 @@ public class StoragePartitionMetaIo extends PartitionMetaIo {
private static final int LEASE_START_TIME_OFF = GC_QUEUE_META_PAGE_ID_OFF
+ Long.BYTES;
+ /** Estimated size here is not a size of a meta, but an approximate rows
count. */
private static final int ESTIMATED_SIZE_OFF = LEASE_START_TIME_OFF +
Long.BYTES;
+ private static final int PRIMARY_REPLICA_NODE_ID_FIRST_PAGE_ID_OFF =
ESTIMATED_SIZE_OFF + Long.BYTES;
Review Comment:
How about inserting these new offsets after `LEASE_START_TIME_OFF`? The
order elsewhere is like this, so:
1. This will be more consistent
2. This might work faster as the fields will be accessed in the same order
in which they are stored (at least, in `initNewPage()`)
##########
modules/table/src/testFixtures/java/org/apache/ignite/distributed/ItTxTestCluster.java:
##########
@@ -713,7 +718,9 @@ public TableViewInternal startTable(String tableName,
SchemaDescriptor schemaDes
catalogService,
schemaManager,
clockServices.get(assignment),
- mock(IndexMetaStorage.class)
+ mock(IndexMetaStorage.class),
+ // TODO use proper index.
Review Comment:
TODOs should have issue references
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/StoragePartitionMeta.java:
##########
@@ -287,10 +299,6 @@ public String toString() {
public void updateLease(@Nullable UUID checkpointId, long leaseStartTime) {
updateSnapshot(checkpointId);
- if (leaseStartTime <= this.leaseStartTime) {
Review Comment:
Why is this removed?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorage.java:
##########
@@ -303,11 +318,50 @@ public void committedGroupConfiguration(byte[] config) {
}
@Override
- public void updateLease(long leaseStartTime) {
+ public void updateLease(
+ long leaseStartTime,
+ String primaryReplicaNodeId,
+ String primaryReplicaNodeName
+ ) {
busy(() -> {
throwExceptionIfStorageNotInRunnableState();
- updateMeta((lastCheckpointId, meta) ->
meta.updateLease(lastCheckpointId, leaseStartTime));
+ updateMeta((lastCheckpointId, meta) -> {
+ primaryReplicaMetaReadWriteLock.writeLock().lock();
+ try {
+ if (leaseStartTime <= meta.leaseStartTime()) {
+ return;
+ }
+
+ if (meta.primaryReplicaNodeIdFirstPageId() ==
BlobStorage.NO_PAGE_ID) {
+ long primaryReplicaNodeIdFirstPageId =
blobStorage.addBlob(stringToBytes(primaryReplicaNodeId));
+
+ meta.primaryReplicaNodeIdFirstPageId(lastCheckpointId,
primaryReplicaNodeIdFirstPageId);
+ } else {
+
blobStorage.updateBlob(meta.primaryReplicaNodeIdFirstPageId(),
stringToBytes(primaryReplicaNodeId));
+ }
+ if (meta.primaryReplicaNodeNameFirstPageId() ==
BlobStorage.NO_PAGE_ID) {
+ long primaryReplicaNodeNameFirstPageId =
blobStorage.addBlob(stringToBytes(primaryReplicaNodeName));
+
+
meta.primaryReplicaNodeNameFirstPageId(lastCheckpointId,
primaryReplicaNodeNameFirstPageId);
+ } else {
+
blobStorage.updateBlob(meta.primaryReplicaNodeNameFirstPageId(),
stringToBytes(primaryReplicaNodeName));
+ }
+
+ meta.updateLease(lastCheckpointId, leaseStartTime);
+
+ this.primaryReplicaNodeId = primaryReplicaNodeId;
+ this.primaryReplicaNodeName = primaryReplicaNodeName;
+ } catch (IgniteInternalCheckedException e) {
+ throw new StorageException(
+ "Cannot save committed group configuration:
[tableId={}, partitionId={}]",
Review Comment:
The error message seems to be a result of a copy paste
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/StoragePartitionMeta.java:
##########
@@ -61,6 +65,8 @@ public class StoragePartitionMeta extends PartitionMeta {
* @param lastAppliedTerm Last applied term value.
* @param lastReplicationProtocolGroupConfigFirstPageId ID of the first
page in a chain storing a blob representing.
* @param leaseStartTime Lease start time.
+ * @param primaryReplicaNodeIdFirstPageId ID of the first page in a chain
storing a blob representing of a primary replica node id.
Review Comment:
```suggestion
* @param primaryReplicaNodeIdFirstPageId ID of the first page in a
chain storing a blob representing a primary replica node id.
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorage.java:
##########
@@ -322,6 +376,74 @@ public long leaseStartTime() {
});
}
+ // TODO https://issues.apache.org/jira/browse/IGNITE-15119 nodeId type
should be changed from String to UUID, after the fix
+ // TODO nodeID will be stored in meta directly and not the blob storage.
+ @Override
+ public @Nullable String primaryReplicaNodeId() {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ try {
+ primaryReplicaMetaReadWriteLock.readLock().lock();
+
+ try {
Review Comment:
Why do we need the external `try`? Can't we move the `catch` logic to the
internal one?
--
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]
