Please elaborate on you preference regarding Objects.requireNonNull()? As currently there are only a few instances of requireNonNull() calls in Drill, why not to fix it now and avoid inconsistency in the future?
Thank you, Vlad > On Aug 22, 2018, at 03:02, Arina Yelchiyeva <arina.yelchiy...@gmail.com> > wrote: > > 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 > <mailto: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 >> <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
