errose28 commented on code in PR #5324:
URL: https://github.com/apache/ozone/pull/5324#discussion_r1337861966
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -378,11 +406,83 @@ public void testMultipleContainerReader() throws
Exception {
" costs " + (System.currentTimeMillis() - startTime) / 1000 + "s");
Assert.assertEquals(containerCount,
containerSet.getContainerMap().entrySet().size());
+ Assert.assertEquals(volumeSet.getFailedVolumesList().size(), 0);
+
+ // One of the conflict01 or conflict02 should have had its container path
+ // removed.
+ List<Path> paths = new ArrayList<>();
+ paths.add(Paths.get(conflict01.getContainerData().getContainerPath()));
+ paths.add(Paths.get(conflict02.getContainerData().getContainerPath()));
+ int exist = 0;
+ for (Path p : paths) {
+ if (Files.exists(p)) {
+ exist++;
+ }
+ }
+ Assert.assertEquals(1, exist);
+ Assert.assertTrue(paths.contains(Paths.get(
+ containerSet.getContainer(0).getContainerData().getContainerPath())));
+
Review Comment:
We should probably check the `containerSet` field in this class as well,
since that is the datanode's actual view of the container state.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -240,6 +249,80 @@ public void verifyAndFixupContainerData(ContainerData
containerData)
}
}
+ private void resolveDuplicate(KeyValueContainer existing,
+ KeyValueContainer toAdd) throws IOException {
+ if (existing.getContainerData().getReplicaIndex() != 0 ||
+ toAdd.getContainerData().getReplicaIndex() != 0) {
+ // This is an EC container. As EC Containers don't have a BSCID, we can't
+ // know which one has the most recent data. Additionally, it is possible
+ // for both copies to have a different replica index for the same
+ // container. Therefore we just let whatever one is loaded first win AND
+ // leave the other one on disk.
+ LOG.warn("Container {} is present at {} and at {}. Both are EC " +
+ "containers. Leaving both containers on disk.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath());
+ return;
+ }
+
+ long existingBCSID = existing.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State existingState
+ = existing.getContainerState();
+ long toAddBCSID = toAdd.getBlockCommitSequenceId();
+ ContainerProtos.ContainerDataProto.State toAddState
+ = toAdd.getContainerState();
+
+ if (existingState != toAddState) {
+ if (existingState == CLOSED) {
+ // If we have mis-matched states, always pick a closed one
+ LOG.warn("Container {} is present at {} with state CLOSED and at " +
+ "{} with state {}. Removing the latter container.",
+ existing.getContainerData().getContainerID(),
+ existing.getContainerData().getContainerPath(),
+ toAdd.getContainerData().getContainerPath(), toAddState);
+ KeyValueContainerUtil.removeContainer(toAdd.getContainerData(),
+ hddsVolume.getConf());
Review Comment:
Do we actually want to remove the container from the disk? It might be safer
just to skip loading it. In the current code a correct delete would have to go
through `KeyValueHandler#deleteInternal` anyways. Just calling this method
could result in the DB entries being removed but the container remaining on
disk, which would show up as corruption on the next startup.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
