You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sumitagrawl (via GitHub)" <gi...@apache.org> on 2025/07/31 16:53:05 UTC
[PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
sumitagrawl opened a new pull request, #8887:
URL: https://github.com/apache/ozone/pull/8887
## What changes were proposed in this pull request?
1. replica index is added in CreateInfo Table along with state, with default value -1
2. during startup for EC container, if replicaIndex do not match with DB (as last loaded index in DN), it will delete that container replica.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13174
## How was this patch tested?
- Unit testcase added
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2267134813
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/WitnessedContainerMetadataStoreImpl.java:
##########
@@ -114,7 +114,7 @@ public void init(DBStore dbStore) throws RocksDatabaseException, CodecException
if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) {
this.containerIdsTable = dbStore.getTable(CONTAINER_IDS_STR_VAL_TABLE, ContainerID.getCodec(),
new DelegatedCodec<>(StringCodec.get(),
- (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal)),
+ (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal), -1),
Review Comment:
added constant INVALID_REPLICA_INDEX and by default when loading data from DB, it will be -1 as can not identify container type as required container data to be loaded.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
+ " deleting the container data.",
+ kvContainer.getContainerData().getContainerID(),
+ kvContainer.getContainerData().getReplicaIndex(),
+ kvContainer.getContainerData().getContainerPath(),
+ containerCreateInfo.getReplicaIndex());
+ KeyValueContainerUtil.removeContainer(kvContainer.getContainerData(), hddsVolume.getConf());
Review Comment:
updated
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2288605499
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -235,6 +238,10 @@ public void verifyAndFixupContainerData(ContainerData containerData)
return;
}
+ if (!isMatchedLastLoadedECContainer(kvContainer, containerSet.getContainerMetadataStore())) {
+ return;
Review Comment:
So are we saying we would not load the container if the replica index doesn't match. This would be wrong since we need to add the container even if the replica index doesn't match. As discussed offline we should add the container to in memory container set irrespective of whether the replica index matches or not and not update the container Set store on disk. I believe it would be good if the logic can be kept inside container set.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,30 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
Review Comment:
Can we move this logic to containerSet class instead of container reader. There is a function containerSet.addContainer(container, boolean overwrite) we could use that if we want to write to disk.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2251895122
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
Review Comment:
The magic number -1 is used to indicate 'no previous replica index information' but should be defined as a constant (e.g., NO_REPLICA_INDEX) to improve code readability and maintainability.
```suggestion
if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() != NO_REPLICA_INDEX
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
Review Comment:
The condition checks for replica index != 0 to identify EC containers, but this magic number should be documented or extracted to a constant to clarify that 0 represents non-EC containers.
```suggestion
if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != NON_EC_CONTAINER_REPLICA_INDEX) {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2297106566
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -235,6 +238,10 @@ public void verifyAndFixupContainerData(ContainerData containerData)
return;
}
+ if (!isMatchedLastLoadedECContainer(kvContainer, containerSet.getContainerMetadataStore())) {
+ return;
Review Comment:
update the conclusion, as this JIRA task is to avoid unknown container load, and there is no chance of having unknown index as check is already present when the container is loaded.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2267134813
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/WitnessedContainerMetadataStoreImpl.java:
##########
@@ -114,7 +114,7 @@ public void init(DBStore dbStore) throws RocksDatabaseException, CodecException
if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) {
this.containerIdsTable = dbStore.getTable(CONTAINER_IDS_STR_VAL_TABLE, ContainerID.getCodec(),
new DelegatedCodec<>(StringCodec.get(),
- (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal)),
+ (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal), -1),
Review Comment:
-1 will represent, no value exist during upgrade case, means any logic should not make any decision.
It will be updated once first time all container is loaded again, so no issue. Differentiation if container is EC or Ratis is required context and this is available only during loading of container which is not available at this point of upgrade (This happens before upgrade).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2251944005
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
Review Comment:
no need fix
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2251943504
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
Review Comment:
no need fix
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2297107102
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,30 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
Review Comment:
This is specific to logic how container will be loaded while startup, not a common logic applicable to other places, so need to be here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2270374099
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
+ " deleting the container data.",
+ kvContainer.getContainerData().getContainerID(),
+ kvContainer.getContainerData().getReplicaIndex(),
+ kvContainer.getContainerData().getContainerPath(),
+ containerCreateInfo.getReplicaIndex());
+ KeyValueContainerUtil.removeContainer(kvContainer.getContainerData(), hddsVolume.getConf());
Review Comment:
As discussion conclusion,
1. Ignore loading EC container if replica index do not match as present in DB. Do not delete the container as decision can not be made considering data loss.
2. while add container to memory, avoid updating container CreateInfo DB if valid replica index is already present, i.e. if -1, update with correct replica index as current behavior. Else avoid updating the table.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl merged PR #8887:
URL: https://github.com/apache/ozone/pull/8887
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2297655204
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,30 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and ignore loading container if not matched.
+ // Ignore matching container replica index -1 in db as no previous replica index
+ if (null != containerCreateInfo
+ && containerCreateInfo.getReplicaIndex() != ContainerCreateInfo.INVALID_REPLICA_INDEX
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2245911663
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -521,6 +546,13 @@ public void testMultipleContainerReader(ContainerTestVersionInfo versionInfo)
assertTrue(Files.exists(Paths.get(ec2.getContainerData().getContainerPath())));
assertNotNull(containerSet.getContainer(3));
+ // For EC conflict with different replica index, one with matching db replica should exist
+ assertTrue(Files.exists(Paths.get(ec3.getContainerData().getContainerPath())));
+ assertTrue(!Files.exists(Paths.get(ec4.getContainerData().getContainerPath())));
+
+ assertTrue(!Files.exists(Paths.get(ec5.getContainerData().getContainerPath())));
Review Comment:
Use assertFalse() instead of assertTrue(!condition) for better readability and clearer failure messages.
```suggestion
assertFalse(Files.exists(Paths.get(ec5.getContainerData().getContainerPath())));
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -521,6 +546,13 @@ public void testMultipleContainerReader(ContainerTestVersionInfo versionInfo)
assertTrue(Files.exists(Paths.get(ec2.getContainerData().getContainerPath())));
assertNotNull(containerSet.getContainer(3));
+ // For EC conflict with different replica index, one with matching db replica should exist
+ assertTrue(Files.exists(Paths.get(ec3.getContainerData().getContainerPath())));
+ assertTrue(!Files.exists(Paths.get(ec4.getContainerData().getContainerPath())));
+
+ assertTrue(!Files.exists(Paths.get(ec5.getContainerData().getContainerPath())));
Review Comment:
Use assertFalse() instead of assertTrue(!condition) for better readability and clearer failure messages.
```suggestion
assertFalse(Files.exists(Paths.get(ec4.getContainerData().getContainerPath())));
assertFalse(Files.exists(Paths.get(ec5.getContainerData().getContainerPath())));
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/ContainerCreateInfo.java:
##########
@@ -47,30 +48,37 @@ public static Codec<ContainerCreateInfo> getNewCodec() {
return CODEC;
}
- private ContainerCreateInfo(ContainerProtos.ContainerDataProto.State state) {
+ private ContainerCreateInfo(ContainerProtos.ContainerDataProto.State state, int replicaIndex) {
this.state = state;
+ this.replicaIndex = replicaIndex;
this.proto = MemoizedSupplier.valueOf(
() -> ContainerProtos.ContainerCreateInfo.newBuilder().setState(state).build());
Review Comment:
The protobuf builder is not setting the replicaIndex field, which means the protobuf representation will always have the default value (-1) regardless of the actual replicaIndex value stored in the object.
```suggestion
() -> ContainerProtos.ContainerCreateInfo.newBuilder()
.setState(state)
.setReplicaIndex(replicaIndex)
.build());
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2266931053
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
+ " deleting the container data.",
+ kvContainer.getContainerData().getContainerID(),
+ kvContainer.getContainerData().getReplicaIndex(),
+ kvContainer.getContainerData().getContainerPath(),
+ containerCreateInfo.getReplicaIndex());
+ KeyValueContainerUtil.removeContainer(kvContainer.getContainerData(), hddsVolume.getConf());
Review Comment:
Let us not remove this upfront. We can remove this replica if another replica with the correct replica index exists. So if the container is already loaded in the memory with incorrect replica index and the correct one gets loaded let us delete the replica then.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/WitnessedContainerMetadataStoreImpl.java:
##########
@@ -114,7 +114,7 @@ public void init(DBStore dbStore) throws RocksDatabaseException, CodecException
if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) {
this.containerIdsTable = dbStore.getTable(CONTAINER_IDS_STR_VAL_TABLE, ContainerID.getCodec(),
new DelegatedCodec<>(StringCodec.get(),
- (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal)),
+ (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal), -1),
Review Comment:
Can we move the -1 to a constant field.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,29 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and delete if not matched, ignoring replica index -1 as no previous replica index information
+ if (null != containerCreateInfo && containerCreateInfo.getReplicaIndex() >= 0
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
+ " deleting the container data.",
+ kvContainer.getContainerData().getContainerID(),
+ kvContainer.getContainerData().getReplicaIndex(),
+ kvContainer.getContainerData().getContainerPath(),
+ containerCreateInfo.getReplicaIndex());
+ KeyValueContainerUtil.removeContainer(kvContainer.getContainerData(), hddsVolume.getConf());
Review Comment:
We should not remove replica index erroneosly since that could lead to error in reconstruction. I believe this can happen during upgrade.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/WitnessedContainerMetadataStoreImpl.java:
##########
@@ -114,7 +114,7 @@ public void init(DBStore dbStore) throws RocksDatabaseException, CodecException
if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) {
this.containerIdsTable = dbStore.getTable(CONTAINER_IDS_STR_VAL_TABLE, ContainerID.getCodec(),
new DelegatedCodec<>(StringCodec.get(),
- (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal)),
+ (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal), -1),
Review Comment:
Shouldn't this be -1 for EC and 0 for ratis containers?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2289776297
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -235,6 +238,10 @@ public void verifyAndFixupContainerData(ContainerData containerData)
return;
}
+ if (!isMatchedLastLoadedECContainer(kvContainer, containerSet.getContainerMetadataStore())) {
+ return;
Review Comment:
As discussed, current addContainer mechanism as implemented to add to DB is not correct, we need change that separately to have rework on previous requirement. So concluded to go with current mechanism.
Solution to go with your comment:
- Need delay addContainer() after all container is loaded in another container Map.
- The while deciding to add to ContainerSet once decision is made
This is refactoring of existing flow of loading container.
May be if there is a concern to allow load of different container is some error scenario of different index, may be we need close this PR till that is completed.
But I do not find any scenario for such thing in healthy dns running.
@swamirishi Share your suggestion if need continue this PR or hold till refactor is completed. IMO, this kind of scenario need manual intervention, but data is available if required manual recover as discussed also.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2289785640
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -235,6 +238,10 @@ public void verifyAndFixupContainerData(ContainerData containerData)
return;
}
+ if (!isMatchedLastLoadedECContainer(kvContainer, containerSet.getContainerMetadataStore())) {
+ return;
Review Comment:
What I was suggesting is in the absence of a replica index matching we want to still support loading one of the containers with non matching replica index. IMO we should atleast load that container in memory by not updating replica index on disk. From what I understand we can still do that without the refactoring. I am not really sure about what refactoring you are talking about.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8887:
URL: https://github.com/apache/ozone/pull/8887#discussion_r2297640975
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -261,6 +268,30 @@ public void verifyAndFixupContainerData(ContainerData containerData)
}
}
+ private boolean isMatchedLastLoadedECContainer(
+ KeyValueContainer kvContainer, WitnessedContainerMetadataStore containerMetadataStore) throws IOException {
+ if (null != containerMetadataStore && kvContainer.getContainerData().getReplicaIndex() != 0) {
+ ContainerCreateInfo containerCreateInfo = containerMetadataStore.getContainerCreateInfoTable()
+ .get(ContainerID.valueOf(kvContainer.getContainerData().getContainerID()));
+ // check for EC container replica index matching if db entry is present for container as last loaded,
+ // and ignore loading container if not matched.
+ // Ignore matching container replica index -1 in db as no previous replica index
+ if (null != containerCreateInfo
+ && containerCreateInfo.getReplicaIndex() != ContainerCreateInfo.INVALID_REPLICA_INDEX
+ && containerCreateInfo.getReplicaIndex() != kvContainer.getContainerData().getReplicaIndex()) {
+ LOG.info("EC Container {} with replica index {} present at path {} is not matched with DB replica index {}," +
Review Comment:
nit: Given that we are not deleting the container data anymore the log should just say not loading the container data.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13174. EC duplicate replica handling for different index in datanodes [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8887:
URL: https://github.com/apache/ozone/pull/8887#issuecomment-3219594978
We can merge the PR after we get a green CI
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org