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.
| blkIDBuilder.setReplicaIndex(replicationIndex); | ||
| } | ||
| // TODO: Replica to the method parameter | ||
| blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK)); |
There was a problem hiding this comment.
Could this stay unset? HDDS-15882 will set the real type at the writeChunkAsync call below, which wouldn't touch this builder, so this line would be easy to leave behind claiming DISK.
| Objects.requireNonNull(data, "data == null"); | ||
| Preconditions.checkState(data.getContainerID() >= 0, "Container Id " + | ||
| "cannot be negative"); | ||
| if (data.getBlockID().getStorageType() != null |
There was a problem hiding this comment.
worth extracting a small helper? PutBlock and PutSmallFile also run it twice, since persistPutBlock repeats it underneath
|
|
||
| @Test | ||
| public void testCreateContainerRejectsInvalidStorageType() throws IOException { | ||
| public void testInvalidStorageTypeDoesNotMarkContainerUnhealthy() throws IOException { |
There was a problem hiding this comment.
With a valid write now preceding it, this exercises the new dispatch-level guard rather than the create-path check in handleCreateContainer. Could the original be kept as a separate test so both paths stay covered?
5 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