HADOOP-17126. implement non-guava Precondition checkNotNull #3050
Conversation
This file contains 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. |
virajjasani
reviewed
May 26, 2021
There was a problem hiding this comment.
Left few comments. Nice one @amahussein, this one is heavily used in the code base.
| @InterfaceAudience.Private | ||
| @InterfaceStability.Unstable | ||
| public final class Validate { | ||
| public static final Logger LOG = |
There was a problem hiding this comment.
nit: private would be better
| public static final Logger LOG = | ||
| LoggerFactory.getLogger(Validate.class); | ||
|
|
||
| public static final String VALIDATE_IS_NOT_NULL_EX_MESSAGE = |
There was a problem hiding this comment.
nit: we can keep default package private scope.
Comment on lines
140
to
151
| String msg = VALIDATE_IS_NOT_NULL_EX_MESSAGE; | ||
| try { | ||
| // note that we can get NPE evaluating the message itself; | ||
| // but we do not want this to override the actual NPE. | ||
| msg = msgSupplier.get(); | ||
| } catch (Exception e) { | ||
| // ideally we want to log the error to capture. This may cause log files | ||
| // to bloat. On the other hand, swallowing the exception may hide a bug | ||
| // in the caller. Debug level is a good compromise between the two | ||
| // concerns. | ||
| LOG.debug("Error formatting message: {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
nit: This one is fine, but how about single assignment? (only if you feel this one is good to use)
String msg;
try {
// note that we can get NPE evaluating the message itself;
// but we do not want this to override the actual NPE.
msg = msgSupplier.get();
} catch (Exception e) {
msg = VALIDATE_IS_NOT_NULL_EX_MESSAGE;
// ideally we want to log the error to capture. This may cause log files
// to bloat. On the other hand, swallowing the exception may hide a bug
// in the caller. Debug level is a good compromise between the two
// concerns.
LOG.debug("Error formatting message: {}", e.getMessage());
}
Comment on lines
30
to
31
| * <p>This class replaces {@code guava.Preconditions} which provides helpers | ||
| * to validate the following conditions: |
There was a problem hiding this comment.
With this, good to mention: Javadocs for majority of APIs in this class are taken from which Guava release version.
There was a problem hiding this comment.
I did not copy the code from guava; but I see your point.
It is going to be helpful to have that added to some APIs. I will review the APIs and see which ones need this info added to their Javadoc.
| try { | ||
| msg = String.format(message, values); | ||
| } catch (Exception e) { | ||
| LOG.debug("Error formatting message: {}", e.getMessage()); |
There was a problem hiding this comment.
As it is already at DEBUG level, do we also want to print stacktrace by providing e as last argument? Perhaps for some hidden case, it might provide good visibility at DEBUG level.
| */ | ||
| @InterfaceAudience.Private | ||
| @InterfaceStability.Unstable | ||
| public final class Validate { |
There was a problem hiding this comment.
If we keep the same name i.e Preconditions, only import will require changes in follow-up PRs and it would look clean.
Given that Preconditions is heavily used in the code base, from change as well as review perspectives, it will be much cleaner. WDYT?
There was a problem hiding this comment.
Sounds like a good idea. I will rename the class.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@busbey Can you please take a look at this PR? |
|
@steveloughran This PR was one of the steps to replace the guava dependency in common module. |
|
steveloughran
approved these changes
Sep 17, 2021
|
|
||
| import org.apache.hadoop.classification.InterfaceAudience; | ||
| import org.apache.hadoop.classification.InterfaceStability; | ||
| import org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting; |
There was a problem hiding this comment.
Do we need this? It makes backporting this patch on its own harder?
There was a problem hiding this comment.
This reminds me of the decision HBase community made sometime back to remove usage of VisibleForTesting entirely from the codebase and replaced them with IA.Private. Here is the commit
now, for the rest of the hadoop-common JIRA, should we create a new JIRA for that and mark the current one as closed (with versions)? I think it'd be best for tracking like that |
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
In order to replace Guava Preconditions, we need to implement our own versions of the API.
This PR is to create
checkNotNullin a new package dubbedunguava.This is splitting the PR that was opened previously #2134
The plan is as follows
package org.apache.hadoop.util.unguava;class Validateorg.apache.hadoop.util.unguava.Validatewith the following interfacecheckNotNull(final T obj)checkNotNull(final T reference, final Object errorMessage)checkNotNull(final T obj, final String message, final Object... values)checkNotNull(final T obj,final Supplier<String> msgSupplier)guava.preconditionsusedString.lenientformatwhich suppressed exceptions caused by string formatting of the exception message . So, in order to avoid changing the behavior, the implementation catches exceptions triggered by building the message (IllegalFormat, InsufficientArg, NullPointer..etc)guava.Preconditions.checkNotNullbyunguava.Validate.checkNotNullSimilar Jiras will be created to implement
checkState,checkArgument,checkIndex