HADOOP-17967. Keep restrict-imports-enforcer-rule for Guava VisibleForTesting in hadoop-main pom#3555
Conversation
…rTesting in hadoop-main pom
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @virajjasani , If we get that PR merged then we will reintroduce the same lines again very soon which is going to be redundant work. |
|
I am fine either way but I think we might have to make this change soon for sure, because not every single pom of all sub-modules are covered, only the ones where VFT is being used are covered. Hence, someone can still technically use Guava provided VFT annotation in any of those modules, it is only after this change we can restrict in every single module just because of the hadoop-main pom, which is directly or indirectly parent pom for every module/sub-module. |
|
We will still have commits landed in the meanwhile and I believe we could consider checking-in this PR. In the end, I think this is the only PR pending for removal of VFT's dependency on Guava, and for Preconditions and any other dependencies on Guava, I think it's still worth adding this enforcer as required to cover particular set of modules/sub-modules so that no two or more enforcer rules becomes mutually dependent on each other, because once we define such inter-dependency, we will again have to be more careful with backports, so it becomes even more complicated. Thoughts @tasanuma @amahussein ? |
|
I wish there was a better way, but I can't think of one. I don't think we can wait too long to merge this PR, because as Viraj said, not every single pom of all sub-modules are covered. |
tasanuma
left a comment
There was a problem hiding this comment.
LGTM. I will merge it tomorrow if there is no objection.
|
Just casually exploring, got curious on how does this work. Checked in the PR branch, got one import Why it is not blocking this & we would need an Addendum somewhere if I am not looking at some old code or have messed up. |
|
Thanks for catching this @ayushtkn. Both checkstyle rules as well as restrict-imports-enforcer maven plugin, focus on imports of specific pattern and produces checkstyle error or build failure based on what we have used from the two of them. However, now I have realized that none of them can detect usage without imports e.g similar to the one you have provided above: I will fix this anyways in the next commit on this PR but how we can restrict such direct usage that don't use imported version of annotation or class or interface might require some brainstorming, no direct technique comes to mind as of now, let me get back if I can find something. |
Thanks @virajjasani , @tasanuma and @ayushtkn |
Sounds good, given that once all usage of guava is removed, it should be anyways removed from the maven dependency. |
|
Shaded guava you can remove, Actual guava I think No. There are transitive dependencies which require guava. |
You are right, have seen this transitive dependency issues with Yarn timelineservice as well as some other modules using Curator dependency. But I think when we get to the point where our codebase is no longer using Guava libraries, we can consider whether |
|
💔 -1 overall
This message was automatically generated. |
tasanuma
left a comment
There was a problem hiding this comment.
The failed tests are not related. +1. It seems there are no objections to this change. I will merge it.
|
Thanks for your contribution, @virajjasani. Thanks for your review and discussion, @amahussein and @ayushtkn. If possible, using guava calls directly should be banned, but since it is a rare case, I don't think it will be much of a problem without it. Maybe we can discuss it in HADOOP-17968. |
…rTesting in hadoop-main pom (apache#3555)
…rTesting in hadoop-main pom (apache#3555)
No description provided.