HADOOP-17952. Replace Guava VisibleForTesting by Hadoop's own annotation in hadoop-common-project modules #3503
Conversation
| * representation will be hard to change. | ||
| */ | ||
| public @interface VisibleForTesting { | ||
| } |
|
Can you make the addition of the new @interface a separate PR, which can be backported trivially. That way we can cherry pick it into older versions with ease |
|
@steveloughran I think backport might be bit painful for this one specifically given that VisibleForTesting is being used at so many places. I was thinking to restrict this to 3.4.0 only. Are you fine with it? Even previous Guava replacement work for |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
+1 |
virajjasani
changed the title
virajjasani
changed the title
virajjasani
force-pushed
the
HADOOP-17947-trunk
branch
from
6ed8b11 to
23aaf38
Compare
|
Updated this PR. Please take a look @tasanuma @amahussein @steveloughran. |
|
🎊 +1 overall
This message was automatically generated. |
| @Target({ ElementType.TYPE, ElementType.METHOD, ElementType.FIELD, | ||
| ElementType.CONSTRUCTOR }) |
There was a problem hiding this comment.
Is this a required change? If so, this should be an addendum PR of HADOOP-17947.
There was a problem hiding this comment.
Yeah you are right indeed. This should go as addendum.
There was a problem hiding this comment.
Is there a need to introduce a new dependency restrict-imports-enforcer-rule ? Hadoop repo used checkStyle import configurations to guard against IllegalImport including illegalPkgs and illegalClasses.
Either way, we should have a single interface to define the list of imports.
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Is this new maven dependency necessary?
AFAIK, hadoop repo used CheckStyle to check for packages/classes.
In hadoop-build-tools/src/main/resources/checkstyle/checkstyle.xml, there is a list of IllegalImport with packages and classes.
I would prefer having a single interface to define IllegalImports. So org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting can be added to heckstyle.xml
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Same as my previous comment. Better to be consistent with other guava banned imports.
@amahussein We are already using this dependency in parent pom. Also, even though checkstyle rule fails, we can't prevent someone from using this import. With this dependency, the build will fail if someone uses this dependency in hadoop-common modules. The eventual goal is to remove project level dependencies and put it only in parent pom as part of final PR. |
Oh! I haven't seen those changes. It was confusing to see that illegal imports are still used in checkstyle.xml . <module name="IllegalImport">
<property name="regexp" value="true"/>
<property name="illegalPkgs" value="sun, com\.google\.common"/>
<property name="illegalClasses" value="^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.io\.BaseEncoding, ^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.base\.(Optional|Function|Predicate|Supplier), ^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.collect\.(ImmutableListMultimap)"/>
</module>
<module name="RedundantImport"/>
<module name="UnusedImports"/>It would be good to have a jira to move the above guava classes to the bannedImports if it is not already there. The checkstyle was doing fine detecting those packages at least for the current phase when we are in the process of defining alternatives. The problem with those pom changes being pushed now is that the guava changes becomes sort of invasive making changes to pom files of each module. Also, since guava replacement commits take time to get merged, we would have individual modules defining their own import rules independently for quite some time. |
Agree, makes sense.
I think since we replace Guava dependencies of specific classes one module at a time, I think it makes sense to push corresponding pom changes for the relevant poms so that in the meantime no one else can commit changes using illegal imports before we are done making changes for all pom modules. And of course, at the end, we will remove plugin from individual modules and put it at parent pom only. Do you not think this is the right approach? Only if we are to make changes for all modules in one shot (one Jira, one PR), we can directly put plugin in parent pom only. Thoughts? |
IMHO the problem here is that if someone has already pushed a couple of commits with illegal imports by the time we are done with all modules, then we will have to take care of such individual modules violating the rule (by replacing the respective imports), it's kind of rework right? |
This process is what we did in HADOOP-17098 to reduce Guava Lists and Guava Sets, and I agree with this approach again. |
Not really, unless the reviewer proceeds with merging the changes ignoring the checkStyle errors, which it is not supposed to happen. Anyway, since #3087 and #3049 are already in, then the way to move forward is 1- to proceed with this PR; 2- to file a new jira to migrate Thanks @virajjasani for the efforts you put into this. Great job! |
Thanks @tasanuma.
Thanks @amahussein, makes sense. Will create Jira to migrate |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @virajjasani ! P.S: I am not sure what the following error is coming from |
virajjasani
force-pushed
the
HADOOP-17947-trunk
branch
from
a93f9f1 to
1a4c427
Compare
…ion in hadoop-common-project modules
virajjasani
force-pushed
the
HADOOP-17947-trunk
branch
from
1a4c427 to
fafbd50
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Probably error on the build side. |
@amahussein sorry, didn't get you. The latest build is already +1, it's with latest push only. https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3503/8/console Build#7 failed due to insufficient rebase but build#8 is the latest after proper rebase with trunk. Do we still want one more build? |
|
I saw the error below but the Details link is not available. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Merged it. Thanks for your contribution, @virajjasani. Thanks for your review, @amahussein. |
|
I'm still not sure if VisibleForTesting should be replaced in the lower branches. If we do it, I think it's ok to do the replacements in all modules at once for the lower branches, instead of proceeding with the replacements in each individual module. |
Sounds good @tasanuma |
+1 |
…ion in hadoop-common-project modules (apache#3503) Reviewed-by: Ahmed Hussein <ahussein@apache.org>
Description of PR
In an attempt to reduce the dependency on Guava, we should remove VisibleForTesting annotation usages as it has very high usage in our codebase. This PR is to provide Hadoop's own alternative and use it in hadoop-common-project modules.
hadoop-auth doesn't have dependency on hadoop-common, hence we attempt to replace VFT annotation with simple comments.
For code changes: