fapifta commented on code in PR #6926:
URL: https://github.com/apache/ozone/pull/6926#discussion_r1674962585
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java:
##########
@@ -65,7 +65,7 @@ public OMRequest validateRequest(OMRequest request)
List<Method> validations = registry.validationsFor(
conditions(request), request.getCmdType(), PRE_PROCESS);
- OMRequest validatedRequest = request.toBuilder().build();
+ OMRequest validatedRequest = request;
Review Comment:
This one is a really good question...
At least this suggestion made me realize that there is a bug that is caused
by the copy itself, as later on when we dispatch the returned validated
request, we dispatch it with the type of the original request, and the traceID
of the original request.
So if someone writes a validator that changes the type of the request, the
dispatching will most likely either fail, or dispatch the request to the wrong
place.
The original intent was to preserve the original request, so that we have it
in case it is needed (if I remember correctly, nothing else comes to my
mind)... but this is really just a speculation, so in light of the possible
issues, and that fact that there does not seem to be a reason for the copy, I
am glad you looked into this and propose this change and the one below for the
response.
One thought, we may just get rid of the validatedRequest and
validatedResponse references, and just use the request and response references
from the parameter list of these methods.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidatorRegistry.java:
##########
@@ -178,13 +179,8 @@ private EnumMap<RequestProcessingPhase, List<Method>>
newPhaseMap() {
return new EnumMap<>(RequestProcessingPhase.class);
}
- private <K, V> V getAndInitialize(K key, V defaultValue, Map<K, V> from) {
- V inMapValue = from.get(key);
- if (inMapValue == null || !from.containsKey(key)) {
- from.put(key, defaultValue);
- return defaultValue;
- }
- return inMapValue;
+ private <K, V> V getAndInitialize(K key, Supplier<V> defaultSupplier, Map<K,
V> from) {
+ return from.computeIfAbsent(key, k -> defaultSupplier.get());
Review Comment:
Probably we can just inline this method, I am not sure why I did not used
this approach before, but this is a great simplification 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: [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]
