adoroszlai commented on code in PR #4990:
URL: https://github.com/apache/ozone/pull/4990#discussion_r1245612933
##########
hadoop-hdds/annotations/src/main/java/org/apache/ozone/annotations/ReplicateAnnotationProcessor.java:
##########
@@ -27,30 +27,48 @@
import javax.lang.model.element.TypeElement;
import javax.lang.model.type.TypeMirror;
import javax.tools.Diagnostic.Kind;
-import java.util.List;
+import java.io.IOException;
+import java.util.Collections;
import java.util.Set;
-import java.util.concurrent.TimeoutException;
+
+import org.apache.commons.lang3.ClassUtils;
+
+import static java.util.stream.Collectors.toSet;
/**
* Annotation Processor that verifies if the methods that are marked with
* Replicate annotation have proper method signature which throws
* TimeoutException.
*/
-@SupportedAnnotationTypes("org.apache.hadoop.hdds.scm.metadata.Replicate")
+@SupportedAnnotationTypes(ReplicateAnnotationProcessor.ANNOTATION_NAME)
@SupportedSourceVersion(SourceVersion.RELEASE_8)
public class ReplicateAnnotationProcessor extends AbstractProcessor {
- private static final String ANNOTATION_SIMPLE_NAME = "Replicate";
+ static final String ANNOTATION_NAME =
+ "org.apache.hadoop.hdds.scm.metadata.Replicate";
+ private static final String REQUIRED_EXCEPTION =
+ "org.apache.hadoop.hdds.scm.exceptions.SCMException";
+
+ private static final Set<String> EXCEPTIONS;
+ static {
+ Set<String> set = ClassUtils.getAllSuperclasses(IOException.class)
Review Comment:
The problem happens when the methods invoked by the proxy (`invokeLocal` or
`invokeRatis`) throw wider exceptions than the annotated method (e.g.
`ContainerStateManager#addContainer`). Thinking in terms of interface and
implementations: specific implementations can throw fewer or more specific
exceptions than those declared by the interface. In this case `invokeRatis` is
the implementation. So I don't think allowing wider exceptions in the
interface is a problem.
To give you a specific example of why we need to allow wider exceptions:
`ContainerStateManager#addContainer` throws `IOException` due to using
`TransactionBuffer`. If we only accept `SCMException` for the annotated
method, it needs to declare both `SCMException` (for `@Replicate`) and
`IOException` (for other reasons) separately.
The problem with wrapping exceptions in `SCMException` is that local and
remote calls may have different results. We can avoid that in two ways:
1. also wrap local exceptions in `SCMException`,
2. instead of wrapping in `SCMException`, require `Exception` to be declared
in the annotated method, translate only invocation-specific exceptions (e.g.
`ExecutionException`) and throw everything else as is.
--
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]
