-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-17152. Provide Hadoop's own Lists utility to reduce dependency on Guava #3061
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
Conversation
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked it against what's currently in master for Guava and LGTM!
I have one comment about the mention of mutability in the classes' comments. Is there an immutable alternative that we can point to? I think from Guava's point of view it makes sense to mention that in the comments since they do offer an immutable alternative, if Hadoop does/doesn't maybe worth specifying?
|
Thanks for the review @bogthe. I understand about providing comments for immutable alternatives but specifically in the case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I myself is not the best reviewer since I seldom to pay attention to helper methods and syntactic sugar, but the PR looks good to me.
Question: now that we have guava shaded and relocated in trunk and branch-3.3, classpath conflict with downstream applications will no longer be an issue. I'm curious if the benefit of maintaining our own Lists utility outweights using Guava
|
@jojochuang HADOOP-17098 provides best reasons for this. Shading Guava in HADOOP-17288 solves the conflicts of Guava library in Hadoop upstream, downstreams, and the other projects (hbase,etc..).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
Merged it. Thanks for your contribution, @virajjasani. Thanks for your reviews, @bogthe and @jojochuang. |
… on Guava (apache#3061) Reviewed-by: Wei-Chiu Chuang <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
… on Guava (#3061) Change-Id: I52e55b9d9826ad661e9ad7dc15f007aa168f0fe1 Reviewed-by: Wei-Chiu Chuang <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
… on Guava (apache#3061) Change-Id: I52e55b9d9826ad661e9ad7dc15f007aa168f0fe1 Reviewed-by: Wei-Chiu Chuang <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
No description provided.