You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/03/04 23:41:21 UTC
[GitHub] [ozone] fapifta commented on a change in pull request #3104: HDDS-6213. Pluggable OzoneManager request handling hooks
fapifta commented on a change in pull request #3104:
URL: https://github.com/apache/ozone/pull/3104#discussion_r819985366
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidationCondition.java
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.om.request.validation;
+
+import org.apache.hadoop.ozone.ClientVersions;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+
+import java.util.function.Predicate;
+
+/**
+ * Defines conditions for which validators can be assigned to.
+ *
+ * These conditions describe a situation where special request handling might
+ * be necessary. In these cases we do not override the actual request handling
+ * code, but based on certain request properties we might reject a request
+ * early, or we might modify the request, or the response received/sent from/to
+ * the client.
+ */
+public enum ValidationCondition {
+ /**
+ * Classifies validations that has to run after an upgrade until the cluster
+ * is in a pre-finalized state.
+ */
+ CLUSTER_NEEDS_FINALIZATION(r -> true,
+ ctx -> ctx.versionManager().needsFinalization()),
+ /**
+ * Classifies validations that has to run, when the client uses an older
+ * protocol version than the server.
+ */
+ OLDER_CLIENT_REQUESTS(r -> r.getVersion() < ClientVersions.CURRENT_VERSION,
+ ctx -> true),
+ /**
+ * Classifies validations that has to run, when the client uses a newer
+ * protocol version than the server.
+ */
+ NEWER_CLIENT_REQUESTS(r -> r.getVersion() > ClientVersions.CURRENT_VERSION,
+ ctx -> true),
+ /**
+ * Classifies validations that has to run for every request.
+ * If you plan to use this, please justify why the validation code should not
+ * be part of the actual request handling code.
+ */
+ UNCONDITIONAL(r -> true, ctx -> true);
Review comment:
This one is kind of a dilemma for me as well, I have removed these two for now, however I started to look into the possibilities of extending the framework to these use cases on the client side, and what changes might be necessary to use a similar approach there and whether it would be beneficial or not... But in case it will happen we can re-introduce the NEWER_CLIENT_REQUESTS, once we start to think about moving out the authorization or audit for example, we can re-introduce UNCONDITIONAL, and if we change both things, we might forget about them :)
Just to do not loose the tests for these enum values, I have just commented out test code, and the enum values, so we can reintroduce them easier if we want.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidationCondition.java
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.om.request.validation;
+
+import org.apache.hadoop.ozone.ClientVersions;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+
+import java.util.function.Predicate;
+
+/**
+ * Defines conditions for which validators can be assigned to.
+ *
+ * These conditions describe a situation where special request handling might
+ * be necessary. In these cases we do not override the actual request handling
+ * code, but based on certain request properties we might reject a request
+ * early, or we might modify the request, or the response received/sent from/to
+ * the client.
+ */
+public enum ValidationCondition {
+ /**
+ * Classifies validations that has to run after an upgrade until the cluster
+ * is in a pre-finalized state.
+ */
+ CLUSTER_NEEDS_FINALIZATION(r -> true,
+ ctx -> ctx.versionManager().needsFinalization()),
+ /**
+ * Classifies validations that has to run, when the client uses an older
+ * protocol version than the server.
+ */
+ OLDER_CLIENT_REQUESTS(r -> r.getVersion() < ClientVersions.CURRENT_VERSION,
+ ctx -> true),
+ /**
+ * Classifies validations that has to run, when the client uses a newer
+ * protocol version than the server.
+ */
+ NEWER_CLIENT_REQUESTS(r -> r.getVersion() > ClientVersions.CURRENT_VERSION,
+ ctx -> true),
+ /**
+ * Classifies validations that has to run for every request.
+ * If you plan to use this, please justify why the validation code should not
+ * be part of the actual request handling code.
+ */
+ UNCONDITIONAL(r -> true, ctx -> true);
+
+ private final Predicate<OMRequest> shouldApplyTo;
+ private final Predicate<ValidationContext> shouldApplyIn;
+
+ ValidationCondition(
+ Predicate<OMRequest> shouldApplyTo,
+ Predicate<ValidationContext> shouldApplyIn) {
+ this.shouldApplyTo = shouldApplyTo;
+ this.shouldApplyIn = shouldApplyIn;
+ }
+
+ boolean shouldApply(OMRequest req, ValidationContext ctx) {
+ return shouldApplyTo.test(req) && shouldApplyIn.test(ctx);
Review comment:
As predicates are type parametrized, we can not use just one predicate that processes either a context or a request, but you are right, it would be way more better to have a more simpler approach to express the condition.
How about using an abstract method and overriding it on a per enum value basis like the one I added instead of the old way?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidatorRegistry.java
##########
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.om.request.validation;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type;
+import org.reflections.Reflections;
+import org.reflections.scanners.MethodAnnotationsScanner;
+import org.reflections.util.ClasspathHelper;
+import org.reflections.util.ConfigurationBuilder;
+
+import java.lang.reflect.Method;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Registry that loads and stores the request validators to be applied by
+ * a service.
+ */
+public class ValidatorRegistry {
+
+ private final EnumMap<
+ ValidationCondition, EnumMap<Type, Pair<List<Method>, List<Method>>>>
+ validators = new EnumMap<>(ValidationCondition.class);
Review comment:
For a condition and a request type there can be pre and post methods also defined, and if so, then both sides of the pair is used. However your understanding is correct, and the approach you are proposing is also way more forward looking as even though we have just two processing phase right now, the structure you are proposing covers any additional processing phase, and does not limit our ability to add a new phase at any time by extending the enum.
I have changed the implementation to use enummap of enummap of enummap instead of enummap of enummap of pairs.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidatorRegistry.java
##########
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.om.request.validation;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type;
+import org.reflections.Reflections;
+import org.reflections.scanners.MethodAnnotationsScanner;
+import org.reflections.util.ClasspathHelper;
+import org.reflections.util.ConfigurationBuilder;
+
+import java.lang.reflect.Method;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Registry that loads and stores the request validators to be applied by
+ * a service.
+ */
+public class ValidatorRegistry {
+
+ private final EnumMap<
+ ValidationCondition, EnumMap<Type, Pair<List<Method>, List<Method>>>>
+ validators = new EnumMap<>(ValidationCondition.class);
+
+ /**
+ * Creates a {@link ValidatorRegistry} instance that discovers validation
+ * methods in the provided package and the packages in the same resource.
+ * A validation method is recognized by the {@link RequestFeatureValidator}
+ * annotation that contains important information about how and when to use
+ * the validator.
+ * @param validatorPackage the main package inside which validatiors should
+ * be discovered.
+ */
+ ValidatorRegistry(String validatorPackage) {
+ this(ClasspathHelper.forPackage(validatorPackage));
+ }
+
+ /**
+ * Creates a {@link ValidatorRegistry} instance that discovers validation
+ * methods under the provided URL.
+ * A validation method is recognized by the {@link RequestFeatureValidator}
+ * annotation that contains important information about how and when to use
+ * the validator.
+ * @param searchUrls the path in which the annotated methods are searched.
+ */
+ ValidatorRegistry(Collection<URL> searchUrls) {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .setUrls(searchUrls)
+ .setScanners(new MethodAnnotationsScanner())
+ .useParallelExecutor()
+ );
+
+ Set<Method> describedValidators =
+ reflections.getMethodsAnnotatedWith(RequestFeatureValidator.class);
+ initMaps(describedValidators);
+ }
+
+ /**
+ * Get the validators that has to be run in the given list of
+ * {@link ValidationCondition}s, for the given requestType and
+ * {@link RequestProcessingPhase}.
+ *
+ * @param conditions conditions that are present for the request
+ * @param requestType the type of the protocol message
+ * @param phase the request processing phase
+ * @return the list of validation methods that has to run.
+ */
+ List<Method> validationsFor(
+ List<ValidationCondition> conditions,
+ Type requestType,
+ RequestProcessingPhase phase) {
+
+ if (conditions.isEmpty() || validators.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ Set<Method> returnValue =
+ new HashSet<>(validationsFor(conditions.get(0), requestType, phase));
Review comment:
You can define the same validator to be used for multiple conditions, as the conditions field in the annotation is defined to be an array. In this case the validator will be added into the list inside both condition for the request type and processing phase.
Also when you are requesting for the validation via this method, you can specify multiple conditions, as the cluster can be in a pre-finalized state when an older client issues a request, and in this case if the same validation is set for both conditions, it would be returned twice, and then the validator would run twice which might be either unnecessary, unexpected or misbehaving the second time.
As you might ask for a use case, I do not have one in mind, but having this in does not seem to be a big price for having this flexibility, though if you think, I am open to drop this, and we can change the conditions to be condition, and let just one condition to be specified in the annotation.
##########
File path: dev-support/annotations/src/main/java/org/apache/ozone/annotations/RequestFeatureValidatorProcessor.java
##########
@@ -0,0 +1,277 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.ozone.annotations;
+
+import javax.annotation.processing.AbstractProcessor;
+import javax.annotation.processing.RoundEnvironment;
+import javax.annotation.processing.SupportedAnnotationTypes;
+import javax.annotation.processing.SupportedSourceVersion;
+import javax.lang.model.SourceVersion;
+import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.AnnotationValue;
+import javax.lang.model.element.AnnotationValueVisitor;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
+import javax.lang.model.type.ExecutableType;
+import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.SimpleAnnotationValueVisitor8;
+import javax.tools.Diagnostic;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.Set;
+
+@SupportedAnnotationTypes(
+ "org.apache.hadoop.ozone.om.request.validation.RequestFeatureValidator")
+@SupportedSourceVersion(SourceVersion.RELEASE_8)
+public class RequestFeatureValidatorProcessor extends AbstractProcessor {
Review comment:
Done, thank you for noting this, altough this indicatest that checkstyle is not running for this module, as the relevant checkstyle rule should have been failed for not having javadoc for a class...
I have looked into this, and added checkstyle checks to the module, also fixed the checkstyle issues discovered.
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/validation/testvalidatorset1/GeneralValidatorsForTesting.java
##########
@@ -0,0 +1,233 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.om.request.validation.testvalidatorset1;
+
+import org.apache.hadoop.ozone.om.request.validation.RequestFeatureValidator;
+import org.apache.hadoop.ozone.om.request.validation.TestRequestValidations;
+import org.apache.hadoop.ozone.om.request.validation.ValidationContext;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.apache.hadoop.ozone.om.request.validation.RequestProcessingPhase.POST_PROCESS;
+import static org.apache.hadoop.ozone.om.request.validation.RequestProcessingPhase.PRE_PROCESS;
+import static org.apache.hadoop.ozone.om.request.validation.ValidationCondition.CLUSTER_NEEDS_FINALIZATION;
+import static org.apache.hadoop.ozone.om.request.validation.ValidationCondition.NEWER_CLIENT_REQUESTS;
+import static org.apache.hadoop.ozone.om.request.validation.ValidationCondition.OLDER_CLIENT_REQUESTS;
+import static org.apache.hadoop.ozone.om.request.validation.ValidationCondition.UNCONDITIONAL;
+import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.CreateKey;
+import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.CreateVolume;
+import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type.DeleteKeys;
+
+/**
+ * Some annotated request validator method, and facilities to help check if
+ * validations were properly called from tests where applicable.
+ */
+public final class GeneralValidatorsForTesting {
+
+ /**
+ * As the validators written here does not override any request or response
+ * but throw exceptions for specific tests, a test that wants to directly
+ * use a validator here, has to turn on this boolean, and the method that
+ * the test relies on has to check for this value.
+ *
+ * This is necessary to do not affect other tests that are testing requests
+ * processing, as for some of those tests this package is on the classpath
+ * and therefore the annotated validations are loadede for them.
+ */
+ private static boolean validatorTestsRunning = false;
+
+ public static void startValidatorTest() {
+ validatorTestsRunning = true;
+ }
+
+ public static void finishValidatorTest() {
+ validatorTestsRunning = false;
+ }
+
+ private GeneralValidatorsForTesting() { }
+
+ /**
+ * Interface to easily add listeners that get notified if a certain validator
+ * method defined in this class was called.
+ *
+ * @see TestRequestValidations for more details on how this intercace is
+ * being used.
+ */
+ @FunctionalInterface
+ public interface ValidationListener {
+ void validationCalled(String calledMethodName);
+ }
+
+ private static List<ValidationListener> listeners = new ArrayList<>();
+
+ public static void addListener(ValidationListener listener) {
+ listeners.add(listener);
+ }
+
+ public static void removeListener(ValidationListener listener) {
+ listeners.remove(listener);
+ }
+
+ private static void fireValidationEvent(String calledMethodName) {
+ listeners.forEach(l -> l.validationCalled(calledMethodName));
+ }
+
+ @RequestFeatureValidator(
+ conditions = { CLUSTER_NEEDS_FINALIZATION },
+ processingPhase = PRE_PROCESS,
+ requestType = CreateKey)
+ public static OMRequest preFinalizePreProcessCreateKeyValidator(
+ OMRequest req, ValidationContext ctx) {
+ fireValidationEvent("preFinalizePreProcessCreateKeyValidator");
+ return req;
+ }
+
+ @RequestFeatureValidator(
+ conditions = { CLUSTER_NEEDS_FINALIZATION },
+ processingPhase = POST_PROCESS,
+ requestType = CreateKey)
+ public static OMResponse preFinalizePostProcessCreateKeyValidator(
Review comment:
As we discussed in one of our meetings, this is an interesting suggestion, and I haven't put too much though in using the annotation on the requests itself before.
If we have the annotation on a request class, then the request type is defined by the request class, so we can skip that, still we remain with a set of methods for four different cases: prefinalize-preprocess, oldclient-preprocess, prefinalize-postprocess, oldclient-postprocess. These methods have to be declared to be used within the annotation, or somewhere. If we want to introduce a new condition or a new processing phase, we would need to introduce more lists for the new cases either in the annotation or elsewhere. An other option might be a naming convention for the methods, but that is a bit harder to check at compile time, and may be error prone.
Pre and post differentiation can not be skipped as the methods for the two has to have a different signature, so it can not fit within the same functional interface. Conditions I believe has to be separated, if not, that would introduce branching in the code of the validator later when any new validation functionality is added.
An alternative for branching when a new validator is added to add multiple validations to a certain case, but in that case the annotation gets bigger and bigger so it becomes harder to see what is happening there.
I know we have mentioned the two phase write request processing as a potential blocker, but all in all that does not seem to be a relevant difference from the point where this framework is integrated to the request processing flow, still I would say it is more beneficial to have small separate methods that do just one thing to be defined as a validator separately, instead of trying to collect all validators in one place in one/multiple annotations before the class declaration, or leaving it to be a potentially growing single method with growing number of code branches, but if you see a potential way of solving this without the mentioned risks, I am open to discuss, I don't see that this change makes life easier or simpler on the long run unfortunately.
--
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