Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HADOOP-17947. Provide alternative to Guava VisibleForTesting #3505

Merged
merged 4 commits into from
Oct 5, 2021

Conversation

virajjasani
Copy link
Contributor

Description of PR

Provide alternative to Guava VisibleForTesting so that we can get rid of Guava dependency to represent testing scope for a class/method/field.

@virajjasani
Copy link
Contributor Author

FYI @amahussein @aajisaka @tasanuma @steveloughran. Once this PR gets in, will update PR #3503 to just use this @interface.
Thanks

@steveloughran
Copy link
Contributor

If we are going to change the javadocs, maybe make clear this is a guava replacement and what guarantees we offer (none). Here's my draft

  /**
   * Annotates a program element that exists, or is more widely visible than
   * otherwise necessary, for the benefits of testability.
   * More specifically <i>testability within the hadoop-* modules</i>.
   * This gives the implicit scope and stability of:
   * <pre>
   * @InterfaceAudience.Private
   * @InterfaceStability.Unstable
   * </pre>
   * If external modules need to access/override these methods, then
   * they MUST be rescoped as public/limited private.
   */

Now, should we actually move this to the package org.apache.hadoop.classification in the module hadoop-annotations? It is a classification, after all. I think from a consistency perspective, it makes sense

@virajjasani
Copy link
Contributor Author

virajjasani commented Oct 1, 2021

@steveloughran Javadoc changes, specifically the relation with IA.Private and IS.Unstable makes the doc look better.

Now, should we actually move this to the package org.apache.hadoop.classification in the module hadoop-annotations? It is a classification, after all. I think from a consistency perspective, it makes sense

This is interesting thought. I think it makes sense. In fact, we also have another advantage here - all modules have dependency on hadoop-annotations but not on hadoop-common so all hadoop-* modules can use it. It's worth moving VFT to hadoop-annotations.

@tasanuma
Copy link
Member

tasanuma commented Oct 4, 2021

Adjust to the VFT in Guava, we may want to annotate it with the following annotations.

@Retention(RetentionPolicy.CLASS)
@Target({ElementType.TYPE, ElementType.METHOD})
@Documented

@virajjasani
Copy link
Contributor Author

Adjust to the VFT in Guava, we may want to annotate it with the following annotations.

@Retention(RetentionPolicy.CLASS)
@Target({ElementType.TYPE, ElementType.METHOD})
@Documented

Thanks @tasanuma. I tried to adjust it to the one defined here https://github.com/google/guava/blob/master/guava/src/com/google/common/annotations/VisibleForTesting.java
But now I see that GwtCompatible annotation itself has the Retention and Target annotations so they should be applied to VFT as well. Let me make this change.

Copy link
Member

@tasanuma tasanuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@virajjasani Thanks for updating it. LGTM.

Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me

@tasanuma tasanuma merged commit 5b1d594 into apache:trunk Oct 5, 2021
@tasanuma
Copy link
Member

tasanuma commented Oct 5, 2021

Merged it. Thanks for your contribution, @virajjasani. Thanks for your review, @steveloughran.

tasanuma pushed a commit that referenced this pull request Oct 5, 2021
Reviewed-by: Steve Loughran <stevel@apache.org>
Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
(cherry picked from commit 5b1d594)
tasanuma pushed a commit that referenced this pull request Oct 5, 2021
Reviewed-by: Steve Loughran <stevel@apache.org>
Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
(cherry picked from commit 5b1d594)
@apache apache deleted a comment from hadoop-yetus Oct 15, 2021
@apache apache deleted a comment from hadoop-yetus Oct 15, 2021
@apache apache deleted a comment from hadoop-yetus Oct 15, 2021
@apache apache deleted a comment from hadoop-yetus Oct 15, 2021
HarshitGupta11 pushed a commit to HarshitGupta11/hadoop that referenced this pull request Nov 28, 2022
…3505)

Reviewed-by: Steve Loughran <stevel@apache.org>
Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
jojochuang pushed a commit to jojochuang/hadoop that referenced this pull request May 23, 2023
…ing (apache#3505)

This includes both the asf commits for this

5b1d594 - HADOOP-17947. Provide alternative to Guava VisibleForTesting (apache#3505)
783e480 - HADOOP-17947. Additional element types for VisibleForTesting (ADDENDUM) (apache#3521)

Reviewed-by: Steve Loughran <stevel@apache.org>
Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
(cherry picked from commit 5b1d594)

(cherry picked from commit 4554833)
(cherry picked from commit 1af94fee55275f0c3c44639b0fa891e5c3e20675)
Signed-off-by: Arpit Agarwal <aagarwal@cloudera.com>

Change-Id: Ice45ed9b1a2988152e041c9063c4a68da9f18f88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants