You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "xichen01 (via GitHub)" <gi...@apache.org> on 2026/09/09 15:39:47 UTC
[PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
xichen01 opened a new pull request, #11223:
URL: https://github.com/apache/ozone/pull/11223
## What changes were proposed in this pull request?
Datanode putBlock Support StroageType
- BlockID add field storageType to record the specific Block replica's storageType on the Datanode.
- putBlock OP will persist the BlockID, so the Block replica's storageType will be persisted.
- writeChunk and putBlock are unified by passing StorageType information in protobuf DatanodeBlockID.
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
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "greenwich (via GitHub)" <gi...@apache.org>.
greenwich commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4002483690
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -195,6 +196,8 @@ public BlockOutputStream(
if (replicationIndex > 0) {
blkIDBuilder.setReplicaIndex(replicationIndex);
}
+ // TODO: Replica to the method parameter
+ blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK));
Review 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`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "anuragp010 (via GitHub)" <gi...@apache.org>.
anuragp010 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3986801170
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
Review 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`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4116049630
##########
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:
Add `StreamInit` and `StreamInitWithPutBlock` to `getBlockID`, so it will check BlockID too.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4135062828
##########
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:
Then could you mark `HddsUtils#getBlockID` and `BlockID#getFromProtobuf` as throwing `IllegalArgumentException`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997035400
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1114,6 +1120,11 @@ ContainerCommandResponseProto handleWriteChunk(
WriteChunkRequestProto writeChunk = request.getWriteChunk();
BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID());
+ if (blockID.getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockID.getStorageType() ==
Review Comment:
I used the existing `canIgnoreException` INVALID_ARGUMENT
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4043316553
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java:
##########
@@ -636,6 +636,7 @@ public static PutSmallFileResponseProto writeSmallFile(
public static void createRecoveringContainer(XceiverClientSpi client,
long containerID, String encodedToken, int replicaIndex)
throws IOException {
+ // TODO StoragePolicy Support EC
Review Comment:
Could you please be more specific and use "EC recovery" instead of just "EC" here
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -195,6 +196,8 @@ public BlockOutputStream(
if (replicationIndex > 0) {
blkIDBuilder.setReplicaIndex(replicationIndex);
}
+ // TODO: Replica to the method parameter
+ blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK));
Review Comment:
Just wondering, should we use the `storageType` from `blockID` here instead of hardcoding StorageType.DISK? Or is that outside the scope of this PR?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java:
##########
@@ -194,6 +195,12 @@ public void buildMissingContainerSetAndValidate(
@Override
public ContainerCommandResponseProto dispatch(
ContainerCommandRequestProto msg, DispatcherContext dispatcherContext) {
+ try {
+ HddsUtils.getBlockID(msg);
+ } catch (IllegalArgumentException e) {
+ return ContainerUtils.logAndReturnError(LOG,
+ new StorageContainerException(e.getMessage(), e, Result.INVALID_ARGUMENT), msg);
+ }
Review Comment:
A minor thought on this design:
1. Calling HddsUtils.getBlockID(msg) feels like a simple getter, but it implicitly performs validation (like checking StorageType) and throws IllegalArgumentException. We might want to consider separating the parsing and validation logic in the future to make the intent clearer.
2. My understanding is that clients typically specify Storage Policies (e.g., hot, warm, cold), while the actual StorageType (e.g., DISK, SSD) is determined and assigned by SCM.If that's the case, an invalid storageTypeID reaching this point feels less like a simple "client sent a bad request" and more like an internal bug. Should we use `Preconditions` instead of catching it and gracefully returning `INVALID_ARGUMENT`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4147171493
##########
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:
Updated
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #11223:
URL: https://github.com/apache/ozone/pull/11223#issuecomment-5790795878
@greenwich @devmadhuu @chungen0126 @ashishkumar50 @anuragp010 PTAL
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4022636814
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -147,12 +174,13 @@ public boolean equals(Object o) {
BlockID blockID = (BlockID) o;
return this.getContainerBlockID().equals(blockID.getContainerBlockID())
&& this.getBlockCommitSequenceId() == blockID.getBlockCommitSequenceId()
- && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex());
+ && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex())
+ && Objects.equals(this.getStorageType(), blockID.getStorageType());
}
@Override
public int hashCode() {
return Objects.hash(containerBlockID.getContainerID(), containerBlockID.getLocalID(),
- blockCommitSequenceId, replicaIndex);
+ blockCommitSequenceId, replicaIndex, storageType);
Review Comment:
Same as above.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -147,12 +174,13 @@ public boolean equals(Object o) {
BlockID blockID = (BlockID) o;
return this.getContainerBlockID().equals(blockID.getContainerBlockID())
&& this.getBlockCommitSequenceId() == blockID.getBlockCommitSequenceId()
- && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex());
+ && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex())
+ && Objects.equals(this.getStorageType(), blockID.getStorageType());
Review Comment:
`storageType` is fine to carry as a field — it's useful for the datanode-side verification in `verifyStorageType()` — but it must not change the notion of what "the same block" means unless storage type is intentionally part of BlockID identity.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "F64116045 (via GitHub)" <gi...@apache.org>.
F64116045 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3986315506
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review 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](https://github.com/google/guava/blob/de1d87eabff93cb4a7fa8e52df5fc46802c95b17/guava/src/com/google/common/base/Preconditions.java#L119-L129), and [IllegalArgumentException extends RuntimeException](https://github.com/openjdk/jdk/blob/jdk-17-ga/src/java.base/share/classes/java/lang/IllegalArgumentException.java#L31-L34). [KeyValueHandler.handle() wraps RuntimeException as CONTAINER_INTERNAL_ERROR](https://github.com/apache/ozone/blob/0b9d22c9f830742ca25cd786b8a8c8c204db8bde/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java#L313-L326), which is not ignored by [HddsDispatcher](https://github.com/apache/ozone/blob/0b9d22c9f830742ca25cd786b8a8c8c204db8bde/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java#L172-L186) when deciding whether to [mark the container unhealthy](https://github.co
m/apache/ozone/blob/0b9d22c9f830742ca25cd786b8a8c8c204db8bde/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java#L406-L435).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3986608812
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1114,6 +1120,11 @@ ContainerCommandResponseProto handleWriteChunk(
WriteChunkRequestProto writeChunk = request.getWriteChunk();
BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID());
+ if (blockID.getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockID.getStorageType() ==
Review Comment:
How about if we add `STORAGE_TYPE_MISMATCH` a dedicated result code ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997018219
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
Review Comment:
Yes, we can add check to handlePutSmallFile.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997030388
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review Comment:
I changed this to return INVALID_ARGUMENT; this is a canIgnoreException.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review Comment:
I changed this to return INVALID_ARGUMENT, this is a canIgnoreException.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4048647281
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -195,6 +196,8 @@ public BlockOutputStream(
if (replicationIndex > 0) {
blkIDBuilder.setReplicaIndex(replicationIndex);
}
+ // TODO: Replica to the method parameter
+ blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK));
Review Comment:
this `storageType` will be set in the future
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu merged PR #11223:
URL: https://github.com/apache/ozone/pull/11223
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4131363977
##########
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:
getBlockID throws an `IllegalArgumentException` (from `BlockID.getFromProtobuf(blockID)`).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4136554040
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
It appears to have been removed, but we can leave it as is for now and remove it later if necessary; since some tests read the block's storage type, we can handle the changes globally.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997030388
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review Comment:
I changed this to return INVALID_ARGUMENT, which is consistent with canIgnoreException
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3986566449
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review 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).
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -187,6 +187,11 @@ public long persistPutBlock(KeyValueContainer container,
Objects.requireNonNull(data, "data == null");
Preconditions.checkState(data.getContainerID() >= 0, "Container Id " +
"cannot be negative");
+ if (data.getBlockID().getStorageType() != null
+ && container.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(data.getBlockID().getStorageType() ==
Review Comment:
same issue here.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1114,6 +1120,11 @@ ContainerCommandResponseProto handleWriteChunk(
WriteChunkRequestProto writeChunk = request.getWriteChunk();
BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID());
+ if (blockID.getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockID.getStorageType() ==
Review Comment:
same comment as above here also.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "greenwich (via GitHub)" <gi...@apache.org>.
greenwich commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4002483690
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -195,6 +196,8 @@ public BlockOutputStream(
if (replicationIndex > 0) {
blkIDBuilder.setReplicaIndex(replicationIndex);
}
+ // TODO: Replica to the method parameter
+ blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK));
Review 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`.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java:
##########
@@ -532,15 +533,20 @@ public void testCreateContainerWithPutSmallFileWithStorageType() throws IOExcept
}
@Test
- public void testCreateContainerRejectsInvalidStorageType() throws IOException {
+ public void testInvalidStorageTypeDoesNotMarkContainerUnhealthy() throws IOException {
Review 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?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -187,6 +188,15 @@ public long persistPutBlock(KeyValueContainer container,
Objects.requireNonNull(data, "data == null");
Preconditions.checkState(data.getContainerID() >= 0, "Container Id " +
"cannot be negative");
+ if (data.getBlockID().getStorageType() != null
Review Comment:
worth extracting a small helper? `PutBlock` and `PutSmallFile` also run it twice, since `persistPutBlock` repeats it underneath
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #11223:
URL: https://github.com/apache/ozone/pull/11223#issuecomment-5868300295
@xichen01 once CI green, I think we can merge the PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4119357553
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
Review Comment:
Added check for getStreamDataChannel
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4134719956
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
@xichen01 Do we need to persist `storageTypeID` in every block record? BlockData uses `getDatanodeBlockIDProtobuf() `for its RocksDB serialization, so this field will be stored in every block entry. Since storage type is a container-level property already persisted in ContainerData/container YAML, and the request value is validated against it before PutBlock persists the block, the per-block persisted copy appears redundant. Could we keep storageTypeID only in the client-to-datanode request and omit it from the RocksDB BlockData codec? If GetBlock needs to return it, it can be populated from the owning container when building the response. This would avoid per-block metadata overhead and the possibility of stale block metadata after container movement.
Based on math: For 1 billion blocks, it will consume roughly 2GB of extra space.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #11223:
URL: https://github.com/apache/ozone/pull/11223#issuecomment-5604608996
@devmadhuu @amaliujia @greenwich @chungen0126 @ivandika3 @peterxcli Please help to review
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4147118922
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
"it appears to have been removed " is a typo, modified.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4118849441
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
Review Comment:
I was wondering if here a valid storagetype in blockId could slip in on mis-matched valid blockId's storageType and container's storageType. Could we validate the storage type here before opening the stream channel? Unlike the normal `WriteChunk` path, this path directly creates the channel without calling `BlockUtils.verifyStorageType().`
For example, an SSD request for an existing DISK container would be accepted because SSD is valid, even though it does not match the container.
If you think, it is being validated and caught anywhere else, please specify , if I am missing something here ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4141867686
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
Thanks, but I am not sure what you are referring to "`it appears to have been removed`". `BlockData.CODEC` still serializes through `BlockData.getProtoBufMessage()` → `BlockID.getDatanodeBlockIDProtobuf()`, so `storageTypeID` is still persisted in the RocksDB block record. So I am assuming that we are planning to handle this in a different PR , so that we keep the field for request-only and update those tests, as I don't see any use case that requires `GetBlock` after restart to return the persisted block-level storage type? If `GetBlock` must
expose it, it could be populated from the owning container when building the response. I have create a separate [JIRA](https://issues.apache.org/jira/browse/HDDS-16652) to track this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4121339763
##########
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:
Do you plan to add other validation here? As `HddsUtils.getBlockID` does not throw exception.
https://github.com/apache/ozone/blob/33913b09971ecad6ed3ee41551707dceea7f70b8/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java#L510
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4119206288
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
Review Comment:
A container's storage type is determined at the time of its creation and specifies the storage type of the DataNode volume where the container is expected to reside; this information is required during container migration, as all blocks within the container move along with it.
Currently, the block's storage type serves only as a metadata field and has no practical function.
Therefore, if a block's storage type differs from the container's storage type, there are no issues—provided the container's storage type is correct.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997029825
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review Comment:
Yes, we can change this, I changed this to return INVALID_ARGUMENT, this is a canIgnoreException.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "greenwich (via GitHub)" <gi...@apache.org>.
greenwich commented on PR #11223:
URL: https://github.com/apache/ozone/pull/11223#issuecomment-5809196915
LGTM
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4136554040
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
It can be removed, but we can leave it as is for now and remove it later if necessary; since some tests read the block's storage type, we can handle the changes globally.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #11223:
URL: https://github.com/apache/ozone/pull/11223#issuecomment-5924642660
Thanks @xichen01 for the continued work on this patch. Thanks @greenwich @peterxcli @anuragp010 for your reviews.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r3997029825
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -692,6 +692,12 @@ ContainerCommandResponseProto handlePutBlock(
ContainerProtos.BlockData data = request.getPutBlock().getBlockData();
BlockData blockData = BlockData.getFromProtoBuf(data);
Objects.requireNonNull(blockData, "blockData == null");
+ Objects.requireNonNull(blockData.getBlockID());
+ if (blockData.getBlockID().getStorageType() != null
+ && kvContainer.getContainerData().getStorageType() != null) {
+ Preconditions.checkArgument(blockData.getBlockID().getStorageType() ==
Review Comment:
Yes, we can change this, I changed this to return INVALID_ARGUMENT, which is consistent with `canIgnoreException`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4018030457
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -187,6 +188,15 @@ public long persistPutBlock(KeyValueContainer container,
Objects.requireNonNull(data, "data == null");
Preconditions.checkState(data.getContainerID() >= 0, "Container Id " +
"cannot be negative");
+ if (data.getBlockID().getStorageType() != null
Review Comment:
Yes, a helper method has already been extracted.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java:
##########
@@ -532,15 +533,20 @@ public void testCreateContainerWithPutSmallFileWithStorageType() throws IOExcept
}
@Test
- public void testCreateContainerRejectsInvalidStorageType() throws IOException {
+ public void testInvalidStorageTypeDoesNotMarkContainerUnhealthy() throws IOException {
Review Comment:
Added
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4143919917
##########
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:
@xichen01 how would you like to handle @peterxcli comment, I think last one open, If this comment gets concluded, we can merge this PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4017792724
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -195,6 +196,8 @@ public BlockOutputStream(
if (replicationIndex > 0) {
blkIDBuilder.setReplicaIndex(replicationIndex);
}
+ // TODO: Replica to the method parameter
+ blkIDBuilder.setStorageTypeID(StorageTypeUtils.getID(StorageType.DISK));
Review Comment:
Both `writeChunkAsync` method and `this.containerBlockData` require a `StorageType` field, which is used for different operations.
You can refer to `replicationIndex`, both `writeChunkAsync` method and `this.containerBlockData` set this value.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4043529362
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java:
##########
@@ -194,6 +195,12 @@ public void buildMissingContainerSetAndValidate(
@Override
public ContainerCommandResponseProto dispatch(
ContainerCommandRequestProto msg, DispatcherContext dispatcherContext) {
+ try {
+ HddsUtils.getBlockID(msg);
+ } catch (IllegalArgumentException e) {
+ return ContainerUtils.logAndReturnError(LOG,
+ new StorageContainerException(e.getMessage(), e, Result.INVALID_ARGUMENT), msg);
+ }
Review Comment:
A minor thought on this design:
1. Calling HddsUtils.getBlockID(msg) feels like a simple getter, but it implicitly performs validation (like checking StorageType) and throws IllegalArgumentException. We might want to consider separating the parsing and validation logic in the future to make the intent clearer.
2. Should we check the storage type here? I believe once we use it and convert proto message to java class it will throw exception.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4116063927
##########
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 added validation in `handleWriteChunk`. We’ve added validation in many places, which tends to bloat the code; since many of these errors would only occur in theory, we should strike a balance.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4048765537
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java:
##########
@@ -194,6 +195,12 @@ public void buildMissingContainerSetAndValidate(
@Override
public ContainerCommandResponseProto dispatch(
ContainerCommandRequestProto msg, DispatcherContext dispatcherContext) {
+ try {
+ HddsUtils.getBlockID(msg);
+ } catch (IllegalArgumentException e) {
+ return ContainerUtils.logAndReturnError(LOG,
+ new StorageContainerException(e.getMessage(), e, Result.INVALID_ARGUMENT), msg);
+ }
Review Comment:
This is to prevent an invalid storage type from causing other exceptions later on, I've moved this code block into a more readable method
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java:
##########
@@ -636,6 +636,7 @@ public static PutSmallFileResponseProto writeSmallFile(
public static void createRecoveringContainer(XceiverClientSpi client,
long containerID, String encodedToken, int replicaIndex)
throws IOException {
+ // TODO StoragePolicy Support EC
Review Comment:
updated
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -147,12 +174,13 @@ public boolean equals(Object o) {
BlockID blockID = (BlockID) o;
return this.getContainerBlockID().equals(blockID.getContainerBlockID())
&& this.getBlockCommitSequenceId() == blockID.getBlockCommitSequenceId()
- && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex());
+ && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex())
+ && Objects.equals(this.getStorageType(), blockID.getStorageType());
}
@Override
public int hashCode() {
return Objects.hash(containerBlockID.getContainerID(), containerBlockID.getLocalID(),
- blockCommitSequenceId, replicaIndex);
+ blockCommitSequenceId, replicaIndex, storageType);
Review Comment:
removed
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -147,12 +174,13 @@ public boolean equals(Object o) {
BlockID blockID = (BlockID) o;
return this.getContainerBlockID().equals(blockID.getContainerBlockID())
&& this.getBlockCommitSequenceId() == blockID.getBlockCommitSequenceId()
- && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex());
+ && Objects.equals(this.getReplicaIndex(), blockID.getReplicaIndex())
+ && Objects.equals(this.getStorageType(), blockID.getStorageType());
Review Comment:
removed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16388. Datanode putBlock Support StorageType [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #11223:
URL: https://github.com/apache/ozone/pull/11223#discussion_r4141867686
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/BlockID.java:
##########
@@ -103,6 +122,9 @@ public ContainerProtos.DatanodeBlockID getDatanodeBlockIDProtobuf() {
if (replicaIndex != null) {
blockID.setReplicaIndex(replicaIndex);
}
+ if (storageType != null) {
+ blockID.setStorageTypeID(StorageTypeUtils.getID(storageType));
Review Comment:
Thanks, but I am not sure what you are referring to "`it appears to have been removed`". `BlockData.CODEC` still serializes through `BlockData.getProtoBufMessage()` → `BlockID.getDatanodeBlockIDProtobuf()`, so `storageTypeID` is still persisted in the RocksDB block record. So I am assuming that we are planning to handle this in a different PR , so that we keep the field for request-only and update those tests, as I don't see any use case that requires `GetBlock` after restart to return the persisted block-level storage type? If `GetBlock` must
expose it, it could be populated from the owning container when building the response. I have created a separate [JIRA](https://issues.apache.org/jira/browse/HDDS-16652) to track this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org