HDDS-10998. Declare annotation processors explicitly

[adoroszlai]

What changes were proposed in this pull request?

Improve Gradle build cache usage by limiting the classpath elements where annotation processors are searched for. This gets rid of warnings emitted during compilation:

[WARNING] The following annotation processors were found on the classpath: ...
Compile avoidance has been deactivated.
...

For more information see https://gradle.com/help/maven-extension-compile-avoidance

(Note: cache is currently only used in local builds, not in CI.)

https://issues.apache.org/jira/browse/HDDS-10998

How was this patch tested?

Local build time is reduced with the patch.

Before:

$ mvn -DskipTests -DskipShade -Dskip.installnpm -Dskip.installnpx -Dskip.npm -Dskip.npx clean verify
...
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:02 min
[INFO] ------------------------------------------------------------------------
[INFO] 659 goals, 646 executed, 13 from cache, saving at least 10s

After:

$ mvn -DskipTests -DskipShade -Dskip.installnpm -Dskip.installnpx -Dskip.npm -Dskip.npx clean verify
...
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  42.801 s
[INFO] ------------------------------------------------------------------------
[INFO] 659 goals, 597 executed, 62 from cache, saving at least 23s

CI: https://github.com/adoroszlai/ozone/actions/runs/9435748229

[fapifta]

Thank you for working on this @adoroszlai, the improvement in build speed looks really great, some 30% is too compelling I would say.

If I understand correctly, this way the selected annotation processors would be used for the given module during compile time only, and I have a concern if this is true. In case of every processor, we have an annotation that they are dealing with, and as I see, the annotation processor is used for only those module where the given annotation is actually used. This may lead us to an easy miss, where an annotation is added to something in a module that does not have the processor set up, as for these annotations we also add some instrumentation elsewhere, the instrumentation may or may not fail, but as we skip the validation there seems to be a possibility to provide an invalid annotation that is processed in some way, and may lead to weird errors. At the moment this is a theoretical case, I do not have an example in mind, I am just curious if you have considered this scenario, and whether this can at all happen... Can you please share your thoughts on this?

[adoroszlai]

Thanks @fapifta for the review.

Annotation processing only happens at compile-time anyway, so there is no change in that.

Before this patch, annotations are processed in hdds-common and all modules that depend on it. It lists all processors we have in Ozone in the javax.annotation.processing.Processor service, and javac finds it on the classpath.

This patch has two parts:

1. replace the ServiceLoader approach with explicit definition in POMs,
2. limit the list of processors, or even disable processing completely, based on each module's needs.

You are right that there is a chance of missing certain processors, either now due to a bug in this patch, or in the future by adding an annotation in some module that previously did not handle that one. This possibility is due to item 2. If we want to avoid it, we can list all supported processors in all modules with any Java code, even if currently unused.

In any case, there is some leftover javadoc in RequestFeatureValidatorProcessor that mentions the ServiceLoader approach. I'll need to update that.

[fapifta]

Thanks for the explanation @adoroszlai!

I can support this approach, however it would be nice to be on the safe side. An idea came to my mind today that might give us some safety... What if we use the plugin that checks for restricted imports and disallow the import of the related annotations where we do not specify the processor for that annotation? This way at least there is a safeguard and anyone who starts to use the annotation or reviews the patch have the chance to notice the missing annotation processor?

[adoroszlai]

use the plugin that checks for restricted imports and disallow the import of the related annotations

@fapifta done, thanks for the idea.

[adoroszlai]

Thanks @fapifta, @kerneltime for the review.