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?

[kerneltime]

why do we need to add the maven-compiler-plugin explicitly in every pom file even when we do not configure the path for an annotation processor?

[kerneltime]

I would guess this is for number 2. limit the list of processors, or even disable processing completely, based on each module's needs.

[adoroszlai]

Yes, this case is for "disable processing".

I guess we could make <proc>none</proc> the default by adding it in root POM, and overriding where necessary.

[adoroszlai]

Unfortunately Java 8 doesn't provide any way to override <proc>none</proc>, because the default ("process and compile") has no corresponding option. Later Java versions can use <proc>full</proc>.

So we can simplify this only after dropping Java 8 support.

[adoroszlai]

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

@fapifta done, thanks for the idea.

[fapifta]

Thank you @adoroszlai for adding the import restrictions, it must have been a tedious work, but at the end of the day it gives us a good safety net on annotation usage within our codebase.
Let's revisit this once we drop Java8 support, should we capture the information regarding the tag somewhere in a TODO comment as part of this change or as a separate follow up JIRA?

[adoroszlai]

Thanks @fapifta, @kerneltime for the review.