Hi All,
My two cents...
The gist of the discussion is that 1) using Objects.checkNotNull() reduces the
Guava import footprint, vs. 2) we are not removing the Guava dependency, so
switching to Objects.checkNotNull() is unnecessary technically and is instead a
personal preference.
We make heavy use of the unique Guava "loading cache". We also use other Guava
preconditions not available in Objects. So deprecation of Guava is unlikely
anytime soon. (Though, doing so would be a GOOD THING, that library causes no
end of grief when importing other libraries due to Guava's habit of removing
features.)
Given that Guava is not going away, I tend to agree with the suggestion there
is no need to do the null check replacement now. It can always be done later
if/when needed.
If we were to want to make the change, I'd suggest we debate preconditions vs.
assert. Drill is now stable, I can't recall a time when I ever saw a
precondition failure in a log file. But, users pay the runtime cost to execute
them zillions of times. At this level of maturity, I'd suggest we use asserts,
which are ignored by the runtime in "non-debug" runs, but which will still
catch failures when we run tests.
Yes, we could argue that the JVM will optimize away the call. But, we do have
code like this, which can't be optimized away:
Preconditions.checkArgument(numSlots <= regionsToScan.size(),
String.format("Incoming endpoints %d is greater than number of scan
regions %d", numSlots, regionsToScan.size()));
So, my suggestion: leave preconditions for now. At some point, open the
assertions vs. preconditions debate.
Thanks,
- Paul
On Tuesday, August 21, 2018, 1:46:10 PM PDT, Vlad Rozov <[email protected]>
wrote:
I am -1 on the first proposal, -0 on the second and +1 for using
Preconditions.checkNotNull() with Objects.requireNonNull() be banned. Please
see [1], [2] (quoted) and [3]:
> Projects which use com.google.common should generally avoid the use of
> Objects.requireNonNull(Object)
> <https://docs.oracle.com/javase/9/docs/api/java/util/Objects.html?is-external=true#requireNonNull-T->.
> Instead, use whichever of checkNotNull(Object)
> <https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html#checkNotNull-T->
> or Verify.verifyNotNull(Object)
> <https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Verify.html#verifyNotNull-T->
> is appropriate to the situation. (The same goes for the message-accepting
> overloads.)
Thank you,
Vlad
[1]
https://stackoverflow.com/questions/34646554/java-util-objects-requirenonnull-vs-preconditions-checknotnull
<https://stackoverflow.com/questions/34646554/java-util-objects-requirenonnull-vs-preconditions-checknotnull>
[2]
https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html
<https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html>
[3] https://github.com/google/guava/wiki/PreconditionsExplained
<https://github.com/google/guava/wiki/PreconditionsExplained> (the last
paragraph)
> On Aug 21, 2018, at 11:50, Vova Vysotskyi <[email protected]> wrote:
>
> Hi all,
>
> I'm working on the PR-1397 <https://github.com/apache/drill/pull/1397>
> for DRILL-6633, which aims to replace usage of Guava classes by JDK ones
> where possible.
>
> One of the things I have done was replacing Preconditions.checkNotNull() by
> Objects.requireNonNull().
>
> I have a discussion in this PR with Vlad Rozov about the replacement
> mentioned above (see our arguments in these comments
> <https://github.com/apache/drill/pull/1397#discussion_r207266884>).
>
> So I'm wondering the opinion of the community regarding the way which
> should be chosen for further development:
> - Should all usages of Preconditions.checkNotNull() be replaced by
> Objects.requireNonNull() and Preconditions.checkNotNull() banned?
> - Should be left Preconditions.checkNotNull() and allowed usage of both
> Preconditions.checkNotNull() and Objects.requireNonNull().
>
> Kind regards,
> Volodymyr Vysotskyi
