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
