HDDS-16388. Datanode putBlock Support StorageType - #11223
Conversation
|
@devmadhuu @amaliujia @greenwich @chungen0126 @ivandika3 @peterxcli Please help to review |
| Objects.requireNonNull(blockData.getBlockID()); | ||
| if (blockData.getBlockID().getStorageType() != null | ||
| && kvContainer.getContainerData().getStorageType() != null) { | ||
| Preconditions.checkArgument(blockData.getBlockID().getStorageType() == |
There was a problem hiding this comment.
Thanks @xichen01, I have a small quesiton.
Could this cause a healthy container to be marked unhealthy when the storage types differ?
Because checkArgument throws IllegalArgumentException when the condition is false, and IllegalArgumentException extends RuntimeException. KeyValueHandler.handle() wraps RuntimeException as CONTAINER_INTERNAL_ERROR, which is not ignored by HddsDispatcher when deciding whether to mark the container unhealthy.
There was a problem hiding this comment.
Yes, we can change this, I changed this to return INVALID_ARGUMENT, this is a canIgnoreException.
| Objects.requireNonNull(blockData.getBlockID()); | ||
| if (blockData.getBlockID().getStorageType() != null | ||
| && kvContainer.getContainerData().getStorageType() != null) { | ||
| Preconditions.checkArgument(blockData.getBlockID().getStorageType() == |
There was a problem hiding this comment.
I think , the mismatch is enforced with Preconditions.checkArgument which throws IllegalArgumentException. That method's try/catch doesn't catch it, so it bubbles up to handle()'s catch (RuntimeException) and becomes CONTAINER_INTERNAL_ERROR. Since that result isn't in canIgnoreException(), the container gets marked UNHEALTHY (and re-replicated).
There was a problem hiding this comment.
I changed this to return INVALID_ARGUMENT, this is a canIgnoreException.
| BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID()); | ||
| if (blockID.getStorageType() != null | ||
| && kvContainer.getContainerData().getStorageType() != null) { | ||
| Preconditions.checkArgument(blockID.getStorageType() == |
There was a problem hiding this comment.
same comment as above here also.
There was a problem hiding this comment.
How about if we add STORAGE_TYPE_MISMATCH a dedicated result code ?
There was a problem hiding this comment.
I used the existing canIgnoreException INVALID_ARGUMENT
| "cannot be negative"); | ||
| if (data.getBlockID().getStorageType() != null | ||
| && container.getContainerData().getStorageType() != null) { | ||
| Preconditions.checkArgument(data.getBlockID().getStorageType() == |
| * using a single RPC. Calls BlockManager and ChunkManager to process the | ||
| * request. | ||
| */ | ||
| ContainerCommandResponseProto handlePutSmallFile( |
There was a problem hiding this comment.
Thanks @xichen01 .
One question: Should we add the storageType validations present in handleWriteChunk and handlePutBlock, to handlePutSmallFile as well? It calls chunkManager.writeChunk directly so the write there isn't validated, though the putBlock flow is covered by the check in persistPutBlock.
There was a problem hiding this comment.
Yes, we can add check to handlePutSmallFile.
4 participants
What changes were proposed in this pull request?
Datanode putBlock Support StroageType
Datanode putBlock Support StroageType
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16388
How was this patch tested?
new test