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