You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Vova Vysotskyi <vv...@gmail.com> on 2018/08/21 18:50:50 UTC
[DISCUSSION] Replacing Preconditions.checkNotNull() with Objects.requireNonNull()
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
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with
Objects.requireNonNull()
Posted by Vlad Rozov <vr...@apache.org>.
Using Java API in case of Objects.requireNonNull() does not help with avoiding dependency on the Guava library and its Preconditions class, so it is only question of consistency. Why to enforce consistency in code style and not to enforce consistency in API used?
Thank you,
Vlad
> On Aug 22, 2018, at 07:09, Arina Yelchiyeva <ar...@gmail.com> wrote:
>
> Obviously, I would intend to use Java API methods in first place since they
> do not introduce extra dependencies. I don't mind the replacements to
> Preconditions for consistency (though it's up the PR author in the scope of
> which Jira that should be done).
>
> Kind regards,
> Arina
>
> On Wed, Aug 22, 2018 at 4:56 PM Vlad Rozov <vr...@apache.org> wrote:
>
>> 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 <ar...@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 <vrozov@apache.org <mailto:
>> vrozov@apache.org>> wrote:
>>>
>>>> My comments inline.
>>>>
>>>> Thank you,
>>>>
>>>> Vlad
>>>>
>>>>
>>>>> On Aug 21, 2018, at 17:05, Paul Rogers <pa...@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 <
>>>> vrozov@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 <vv...@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
>>
>>
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with Objects.requireNonNull()
Posted by Arina Yelchiyeva <ar...@gmail.com>.
Obviously, I would intend to use Java API methods in first place since they
do not introduce extra dependencies. I don't mind the replacements to
Preconditions for consistency (though it's up the PR author in the scope of
which Jira that should be done).
Kind regards,
Arina
On Wed, Aug 22, 2018 at 4:56 PM Vlad Rozov <vr...@apache.org> wrote:
> 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 <ar...@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 <vrozov@apache.org <mailto:
> vrozov@apache.org>> wrote:
> >
> >> My comments inline.
> >>
> >> Thank you,
> >>
> >> Vlad
> >>
> >>
> >>> On Aug 21, 2018, at 17:05, Paul Rogers <pa...@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 <
> >> vrozov@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 <vv...@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
>
>
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with
Objects.requireNonNull()
Posted by Vlad Rozov <vr...@apache.org>.
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 <ar...@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 <vrozov@apache.org <ma...@apache.org>> wrote:
>
>> My comments inline.
>>
>> Thank you,
>>
>> Vlad
>>
>>
>>> On Aug 21, 2018, at 17:05, Paul Rogers <pa...@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 <
>> vrozov@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 <vv...@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
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with Objects.requireNonNull()
Posted by Arina Yelchiyeva <ar...@gmail.com>.
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 <vr...@apache.org> wrote:
> My comments inline.
>
> Thank you,
>
> Vlad
>
>
> > On Aug 21, 2018, at 17:05, Paul Rogers <pa...@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 <
> vrozov@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 <vv...@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
>
>
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with
Objects.requireNonNull()
Posted by Vlad Rozov <vr...@apache.org>.
My comments inline.
Thank you,
Vlad
> On Aug 21, 2018, at 17:05, Paul Rogers <pa...@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 <vr...@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 <vv...@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
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with
Objects.requireNonNull()
Posted by Paul Rogers <pa...@yahoo.com.INVALID>.
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 <vr...@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 <vv...@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
Re: [DISCUSSION] Replacing Preconditions.checkNotNull() with
Objects.requireNonNull()
Posted by Vlad Rozov <vr...@apache.org>.
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 <vv...@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