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: [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]
