devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4092545991
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java:
##########
@@ -556,6 +562,15 @@ private void validateToken(
);
}
+ private static void validateBlockID(ContainerCommandRequestProto msg)
+ throws StorageContainerException {
+ try {
+ HddsUtils.getBlockID(msg);
Review Comment:
I think this does not handle `StreamInit` or `StreamInitWithPutBlock`, even
though both commands carry their block ID inside `WriteChunk` so `StreamInit`
can still mark a healthy container unhealthy ?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1114,6 +1116,7 @@ ContainerCommandResponseProto handleWriteChunk(
WriteChunkRequestProto writeChunk = request.getWriteChunk();
BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID());
+ BlockUtils.verifyStorageType(kvContainer.getContainerData(), blockID);
Review Comment:
I think this validates only the outer `writeChunk.blockID`. When
`writeChunk.hasBlock() `is true, the request contains a second independently
supplied ID at `writeChunk.block.blockData.blockID.`. The embedded ID is
currently parsed only after `chunkManager.writeChunk()`:
Its storage type is then validated later by
`BlockManagerImpl.persistPutBlock(),` after `chunkManager.writeChunk() `and,
for EOF requests, after `finishWriteChunks()`. The outer validation succeeds,
chunk processing occurs, and only then does the embedded `PutBlock` fail with
INVALID_ARGUMENT. This can leave chunk-side effects without the corresponding
block metadata being persisted. Similarly, an invalid embedded value for
`storageTypeID` is converted after chunk processing and may become
`CONTAINER_INTERNAL_ERROR` instead of a clean `INVALID_ARGUMENT`.
Could we parse and validate the embedded BlockData before
`chunkManager.writeChunk()` whenever `writeChunk.hasBlock() `is true? At
minimum, its storage type should be validated against the container, and the
embedded block identity should be checked against the outer block ID before any
mutation. Please also add a test with mismatched/invalid outer and embedded
storage types that verifies:
1. the request returns `INVALID_ARGUMENT`;
2. no chunk or block metadata side effects occur; and
3. the container remains `OPEN`.
--
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]
