HADOOP-17123. remove guava Preconditions from Hadoop-common module #2134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Jul 13, 2020
There was a problem hiding this comment.
This is going to be a big piece of work. not just the coding, but the merging and backporting.
- we are going to have to backport the noguava package back many versions of releases, so that then cherrypicking is easy. That doesn't need any other changes to go...it should be a separate patch
- I'm not sure about "noguava" as a name. "unguava"?
- package must be scoped private/unstable
- we need separate checks for state and arg, as different exceptions are raised.
Also, if we are going to do this, can we have one overload which takes a Supplier<String> messageSupplier as Objects.requireNonNull() does. Gives us room to play in future
Finally, big thing to consider: we need tests for the methods actually doing what we expect. That's raising exceptions and forming the correct strings.
| super(); | ||
| } | ||
|
|
||
| public static void checkIsTrue(final boolean expression, final String message, |
There was a problem hiding this comment.
its explicitly an argument here, hence the IllegalArgumentException.
We will also need checkState, which throw ome illegal state exception too.
We need both and so the check method names should be distinct
There was a problem hiding this comment.
This is going to be a big piece of work. not just the coding, but the merging and backporting.
- we are going to have to backport the noguava package back many versions of releases, so that then cherrypicking is easy. That doesn't need any other changes to go...it should be a separate patch
- I'm not sure about "noguava" as a name. "unguava"?
- package must be scoped private/unstable
- we need separate checks for state and arg, as different exceptions are raised.
Also, if we are going to do this, can we have one overload which takes a
Supplier<String> messageSupplieras Objects.requireNonNull() does. Gives us room to play in futureFinally, big thing to consider: we need tests for the methods actually doing what we expect. That's raising exceptions and forming the correct strings.
Thanks @steveloughran for the feedback. I created a new PR #2143 that has unguava.Validate#checkNotNull implementation.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
revisiting this...we should get this in so it can be backported and we can keep guava out of future patches. is the name 'noguava' the right one?
|
|
(oh, and for merging, I'd propose two commits: one to create the new Predicate class, one to use). Why so? Makes it straightforward to cherrypick the predicate class across all old versions so that other cherrypicked code can use it, without worrying about the big merge) |
|
Thanks @steveloughran |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute