I don't feel like banning Objects.checkNotNull() right now though even Guava suggests not using this method. I suggest we leave it as is in the scope of current PR#1397 (revert replacements where they were done) and discuss further approach on how we should treat checks at runtime.
Kind regards, Arina On Wed, Aug 22, 2018 at 5:32 AM Vlad Rozov <vro...@apache.org> wrote: > My comments inline. > > Thank you, > > Vlad > > > > On Aug 21, 2018, at 17:05, Paul Rogers <par0...@yahoo.com.INVALID> > wrote: > > > > 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. > > The gist of the discussion in the PR and on the mailing list is whether or > not to use a functionality(methods) provided by a library (in this case > Guava) that is(are) also available in JDK or it(they) needs to be > pro-actively replaced by the functionality(methods) provided by the JDK. My > take is that it will be justified only in case the entire dependency on the > library can be eliminated or when the library author deprecated the > functionality(methods) in use. It is not the case for Guava library and > Preconditions class it provides. > > Guava explicitly recommends to avoid using Objects.checkNotNull() method, > so I suggested to prohibit it’s usage as 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.) > > There is a separate PR that takes care of the “grief when importing other > libraries" that also depend on Guava caused by “Guava’s habit of removing > features”. Additionally, Guava is mostly source compatible across version > since version 21.0 (see > https://github.com/google/guava/blob/master/README.md < > https://github.com/google/guava/blob/master/README.md>), so I highly > doubt that dependency on Guava will ever go away. > > > > > 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. > > Actually, asserts are *not* ignored by the runtime in “non-debug” runs, > they may be optimized away by the hotspot compiler. Additionally, I will be > really surprised to see that replacing preconditions with assert will save > more time in all customer runs compared to how long it will take to discuss > the change, make and merge it. > > > > > 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())); > > This is a bad example of using Preconditions. It needs to be changed to > > Preconditions.checkArgument(numSlots <= regionsToScan.size(), > "Incoming endpoints %s is greater than number of scan regions %s”, > numSlots, regionsToScan.size()); > > that will be inlined by the hotspot compiler. > > > > > > > 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 < > vro...@apache.org> 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 <vvo...@gmail.com> 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 > >
