devmadhuu commented on code in PR #10702:
URL: https://github.com/apache/ozone/pull/10702#discussion_r3556455859
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -103,6 +103,12 @@ public final class OzoneConsts {
public static final int CHUNK_SIZE = 1 * 1024 * 1024; // 1 MB
// for client and DataNode to label a block contains a incremental chunk
list.
public static final String INCREMENTAL_CHUNK_LIST = "incremental";
+ /**
+ * Write metadata: when value is {@link #CONTAINER_CREATABLE_FALSE}, the
DataNode must not
+ * auto-create a missing container on WriteChunk (EC reconstruction writes).
+ */
+ public static final String CONTAINER_CREATABLE = "containerCreatable";
+ public static final String CONTAINER_CREATABLE_FALSE = "false";
Review Comment:
```suggestion
public static final String CONTAINER_CREATABLE = "false";
```
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java:
##########
@@ -90,6 +90,14 @@ public ECBlockOutputStream(
this.datanodeDetails = pipeline.getClosestNode();
}
+ @Override
+ protected ChunkInfo.Builder decorateChunkInfo(ChunkInfo.Builder builder) {
Review Comment:
I think, this might break the normal EC write as well because
`ECBlockOutputStream` is used by both the reconstruction coordinator
(`ECReconstructionCoordinator`) and the normal client write path
(`ECBlockOutputStreamEntry`). So we might need to have this flag
`CONTAINER_CREATABLE_FALSE` only for reconstruction path.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java:
##########
@@ -403,4 +403,24 @@ public static long getPendingDeletionBytes(ContainerData
containerData) {
" not support.");
}
}
+
+ /**
+ * @return true if the DataNode may auto-create a missing container for this
WriteChunk request
+ */
+ public static boolean isContainerCreatable(ContainerCommandRequestProto
request) {
Review Comment:
This flag is currently only restricted to check `writeChunk` metadata only,
but missing `PutBlock / PutSmallFile`check. In a partial/padding stripe,
non-writing nodes do create containers by an empty `PutBlock`, so in normal EC
writepath, it is fine. But in case of reconstruction path, a slight edge case
where `PutBlock`
might land up on a force-deleted RECOVERING container can still auto-create
it as OPEN
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##########
@@ -440,6 +440,16 @@ public boolean removeRecoveringContainer(long containerId)
{
return false;
}
+ /**
+ * Reset the stale recovering scrub deadline for an active RECOVERING
container.
+ */
+ public void updateRecoveringContainerTimeout(long containerId) {
+ Preconditions.checkState(containerId >= 0, "Container Id cannot be
negative.");
+ removeRecoveringContainer(containerId);
Review Comment:
This could be a performance issue, earlier `removeRecoveringContainer` was
called when container should be marked as `CLOSE`, but now we are calling this
a very hot path for every chunk write. Number of RECOVERING containers itself
may be less, but number of chunks in lot of RECOVERING containers will add up
this method call multiple times and this `removeRecoveringContainer` method is
iterating all containers by value of hashmap entry. One possible solution: How
about storing the recovering containers using `containerId` as key in hashmap
and deadline time as value, then it will be `O(1)` operation
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1105,9 +1105,18 @@ ContainerCommandResponseProto handleWriteChunk(
request);
}
+ maybeUpdateRecoveringContainerTimeout(kvContainer);
Review Comment:
There could be a possibility of race condition still when
[getTasks](https://github.com/apache/ozone/blob/199da36ad64dbf7941b1c0c3db6e8f15d647551b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/StaleRecoveringContainerScrubbingService.java#L57)
check here if deadline crossed and then adds to `backgroundTaskQueue`, but by
the time queue picks up and task call `markContainerUnhealthy`, a writeChunk
call of this method may adds a fresh deadline entry but the already-queued task
still marks the container UNHEALTHY — and leaves a stale entry for a
now-UNHEALTHY container.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -907,11 +907,11 @@ private CompletableFuture<PutBlockResult>
writeChunkToContainer(
ChecksumData checksumData = checksum.computeChecksum(chunk, false);
// side note: checksum object is shared with PutBlock's (blockData)
checksum calc,
// current impl does not support caching both
- ChunkInfo chunkInfo = ChunkInfo.newBuilder()
+ ChunkInfo chunkInfo = decorateChunkInfo(ChunkInfo.newBuilder()
.setChunkName(blockID.get().getLocalID() + "_chunk_" + ++chunkIndex)
Review Comment:
`decorateChunkInfo` method will fall into both normal EC write path and
reconstruction path. Kindly check this part.
--
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]
