You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Eugene Kirpichov <ki...@google.com.INVALID> on 2017/08/10 23:59:07 UTC
Re: Style of messages for checkArgument/checkNotNull in IOs
https://beam.apache.org/contribute/ptransform-style-guide/#validation now
includes the new guidance.
It also includes updated guidance on what to put in expand() vs. validate()
(TL;DR: validate() is almost always unnecessary. Put almost all validation
in expand())
On Fri, Jul 28, 2017 at 11:56 AM Kenneth Knowles <kl...@google.com.invalid>
wrote:
> So here's an easy solution: our own checkNotNull that throws
> InvalidArgumentException with a good error message. The error message can
> then be templated to allow terse invocations with just the method and
> parameter.
>
> Unsure why I didn't go for this straightaway.
>
> On Fri, Jul 28, 2017 at 11:43 AM, Reuven Lax <re...@google.com.invalid>
> wrote:
>
> > Yuck. I think that checkNotNull throwing a NPE is a very poor design
> choice
> > from the author of checkNotNull.
> >
> > On Fri, Jul 28, 2017 at 11:29 AM, Kenneth Knowles <klk@google.com.invalid
> >
> > wrote:
> >
> > > As to the choice of check method:
> > >
> > > - checkArgument throws an InvalidArgumentException, which is clearly
> in
> > > "your fault" category, a la HTTP 4xx
> > > - checkNotNull throws an NPE, which is usually a "my fault"
> exception, a
> > > la HTTP 5xx.
> > >
> > > The docs on NPE are not clear on blame, and this is a bug in the docs.
> > But
> > > almost all the time, an NPE indicates that the line where it is thrown
> is
> > > incorrect. InvalidArgumentException is unambiguous. This could also be
> > > called a bug in checkNotNull. It throws the same exception as if you
> > > _forgot_ to check if it was not null. So it sort of doesn't do one of
> > the
> > > most important things it should be doing.
> > >
> > > As to verbosity: All error messages should be actionable. We have a
> > chronic
> > > problem with terrible or nonexistent error messages.
> > >
> > > NPE is uninformative and this feeds into the prior two bullets: If I
> see
> > > "NPE on line XYZ of file ABC" I am _always_ going to file a bug against
> > the
> > > author of file ABC because they dereferenced null. Their fix might be
> to
> > > simply protect themselves with a checkArgument to clearly blame their
> > > caller, but that is a totally acceptable bug/fix pair.
> > >
> > > We should really get an analysis in place based on @Nullable
> annotations
> > to
> > > mitigate this a bit, too.
> > >
> > > Kenn
> > >
> > > On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
> > > kirpichov@google.com.invalid> wrote:
> > >
> > > > Hey all,
> > > >
> > > > I think this has been discussed before on a JIRA issue but I can't
> find
> > > it,
> > > > so raising again on the mailing list.
> > > >
> > > > Various IO (and non-IO) transforms validate their builder parameters
> > > using
> > > > Preconditions.checkArgument/checkNotNull, and use different styles
> for
> > > > error messages. There are 2 major styles:
> > > >
> > > > 1) style such as:
> > > > checkNotNull(username, "username"), or checkArgument(username !=
> null,
> > > > "username can not be null") or checkArgument(username != null,
> > > > "username must be set");
> > > > checkArgument(batchSize > 0, "batchSize must be non-negative, but
> was:
> > > %s",
> > > > batchSize)
> > > >
> > > > 2) style such as:
> > > > checkArgument(
> > > > username != null,
> > > > "ConnectionConfiguration.create().withBasicCredentials(
> > > > username,
> > > > password) "
> > > > + "called with null username");
> > > > checkArgument(
> > > > !username.isEmpty(),
> > > > "ConnectionConfiguration.create().withBasicCredentials(
> > > > username,
> > > > password) "
> > > > + "called with empty username");
> > > >
> > > > Style 2 is recommended by the PTransform Style Guide
> > > >
> https://beam.apache.org/contribute/ptransform-style-guide/#transform-
> > > > configuration-errors
> > > >
> > > > However:
> > > > 1) The usage of these two styles is not consistent - both are used in
> > > about
> > > > the same amounts in Beam IOs.
> > > > 2) Style 2 seems unnecessarily verbose to me. The exception thrown
> > from a
> > > > checkArgument or checkNotNull already includes the method being
> called
> > > into
> > > > the stack trace, so I don't think the message needs to include the
> > > method.
> > > > 3) Beam is not the first Java project to have validation of
> > configuration
> > > > parameters of something or another, and I don't think I've seen
> > something
> > > > as verbose as style 2 used anywhere else in my experience of writing
> > > Java.
> > > >
> > > > What do people think about changing the guidance in favor of style 1?
> > > >
> > > > Specifically change the following example:
> > > >
> > > > public Twiddle withMoo(int moo) {
> > > > checkArgument(moo >= 0 && moo < 100,
> > > > "Thumbs.Twiddle.withMoo() called with an invalid moo of %s. "
> > > > + "Valid values are 0 (inclusive) to 100 (exclusive)",
> > > > moo);
> > > > return toBuilder().setMoo(moo).build();}
> > > >
> > > > into the following:
> > > >
> > > > public Twiddle withMoo(int moo) {
> > > > checkArgument(moo >= 0 && moo < 100,
> > > > "Valid values for moo are 0 (inclusive) to 100 (exclusive), "
> > > > + "but was: %s",
> > > > moo);
> > > > return toBuilder().setMoo(moo).build();}
> > > >
> > > >
> > > > And in simpler cases such as non-null checks:
> > > > public Twiddle withUsername(String username) {
> > > > checkNotNull(username, "username");
> > > > checkArgument(!username.isEmpty(), "username can not be empty");
> > > > ...
> > > > }
> > > >
> > >
> >
>
Re: Style of messages for checkArgument/checkNotNull in IOs
Posted by Robert Bradshaw <ro...@google.com.INVALID>.
Huge +1 to the
checkArgument(username != null, ...)
style. A note on validate(), aren't we trying to remove pipeline
options from PTransforms altogether (and, in addition, how does this
even work with the Runner API and cross-language transforms).
On Thu, Aug 10, 2017 at 4:59 PM, Eugene Kirpichov
<ki...@google.com.invalid> wrote:
> https://beam.apache.org/contribute/ptransform-style-guide/#validation now
> includes the new guidance.
>
> It also includes updated guidance on what to put in expand() vs. validate()
> (TL;DR: validate() is almost always unnecessary. Put almost all validation
> in expand())
>
> On Fri, Jul 28, 2017 at 11:56 AM Kenneth Knowles <kl...@google.com.invalid>
> wrote:
>
>> So here's an easy solution: our own checkNotNull that throws
>> InvalidArgumentException with a good error message. The error message can
>> then be templated to allow terse invocations with just the method and
>> parameter.
>>
>> Unsure why I didn't go for this straightaway.
>>
>> On Fri, Jul 28, 2017 at 11:43 AM, Reuven Lax <re...@google.com.invalid>
>> wrote:
>>
>> > Yuck. I think that checkNotNull throwing a NPE is a very poor design
>> choice
>> > from the author of checkNotNull.
>> >
>> > On Fri, Jul 28, 2017 at 11:29 AM, Kenneth Knowles <klk@google.com.invalid
>> >
>> > wrote:
>> >
>> > > As to the choice of check method:
>> > >
>> > > - checkArgument throws an InvalidArgumentException, which is clearly
>> in
>> > > "your fault" category, a la HTTP 4xx
>> > > - checkNotNull throws an NPE, which is usually a "my fault"
>> exception, a
>> > > la HTTP 5xx.
>> > >
>> > > The docs on NPE are not clear on blame, and this is a bug in the docs.
>> > But
>> > > almost all the time, an NPE indicates that the line where it is thrown
>> is
>> > > incorrect. InvalidArgumentException is unambiguous. This could also be
>> > > called a bug in checkNotNull. It throws the same exception as if you
>> > > _forgot_ to check if it was not null. So it sort of doesn't do one of
>> > the
>> > > most important things it should be doing.
>> > >
>> > > As to verbosity: All error messages should be actionable. We have a
>> > chronic
>> > > problem with terrible or nonexistent error messages.
>> > >
>> > > NPE is uninformative and this feeds into the prior two bullets: If I
>> see
>> > > "NPE on line XYZ of file ABC" I am _always_ going to file a bug against
>> > the
>> > > author of file ABC because they dereferenced null. Their fix might be
>> to
>> > > simply protect themselves with a checkArgument to clearly blame their
>> > > caller, but that is a totally acceptable bug/fix pair.
>> > >
>> > > We should really get an analysis in place based on @Nullable
>> annotations
>> > to
>> > > mitigate this a bit, too.
>> > >
>> > > Kenn
>> > >
>> > > On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
>> > > kirpichov@google.com.invalid> wrote:
>> > >
>> > > > Hey all,
>> > > >
>> > > > I think this has been discussed before on a JIRA issue but I can't
>> find
>> > > it,
>> > > > so raising again on the mailing list.
>> > > >
>> > > > Various IO (and non-IO) transforms validate their builder parameters
>> > > using
>> > > > Preconditions.checkArgument/checkNotNull, and use different styles
>> for
>> > > > error messages. There are 2 major styles:
>> > > >
>> > > > 1) style such as:
>> > > > checkNotNull(username, "username"), or checkArgument(username !=
>> null,
>> > > > "username can not be null") or checkArgument(username != null,
>> > > > "username must be set");
>> > > > checkArgument(batchSize > 0, "batchSize must be non-negative, but
>> was:
>> > > %s",
>> > > > batchSize)
>> > > >
>> > > > 2) style such as:
>> > > > checkArgument(
>> > > > username != null,
>> > > > "ConnectionConfiguration.create().withBasicCredentials(
>> > > > username,
>> > > > password) "
>> > > > + "called with null username");
>> > > > checkArgument(
>> > > > !username.isEmpty(),
>> > > > "ConnectionConfiguration.create().withBasicCredentials(
>> > > > username,
>> > > > password) "
>> > > > + "called with empty username");
>> > > >
>> > > > Style 2 is recommended by the PTransform Style Guide
>> > > >
>> https://beam.apache.org/contribute/ptransform-style-guide/#transform-
>> > > > configuration-errors
>> > > >
>> > > > However:
>> > > > 1) The usage of these two styles is not consistent - both are used in
>> > > about
>> > > > the same amounts in Beam IOs.
>> > > > 2) Style 2 seems unnecessarily verbose to me. The exception thrown
>> > from a
>> > > > checkArgument or checkNotNull already includes the method being
>> called
>> > > into
>> > > > the stack trace, so I don't think the message needs to include the
>> > > method.
>> > > > 3) Beam is not the first Java project to have validation of
>> > configuration
>> > > > parameters of something or another, and I don't think I've seen
>> > something
>> > > > as verbose as style 2 used anywhere else in my experience of writing
>> > > Java.
>> > > >
>> > > > What do people think about changing the guidance in favor of style 1?
>> > > >
>> > > > Specifically change the following example:
>> > > >
>> > > > public Twiddle withMoo(int moo) {
>> > > > checkArgument(moo >= 0 && moo < 100,
>> > > > "Thumbs.Twiddle.withMoo() called with an invalid moo of %s. "
>> > > > + "Valid values are 0 (inclusive) to 100 (exclusive)",
>> > > > moo);
>> > > > return toBuilder().setMoo(moo).build();}
>> > > >
>> > > > into the following:
>> > > >
>> > > > public Twiddle withMoo(int moo) {
>> > > > checkArgument(moo >= 0 && moo < 100,
>> > > > "Valid values for moo are 0 (inclusive) to 100 (exclusive), "
>> > > > + "but was: %s",
>> > > > moo);
>> > > > return toBuilder().setMoo(moo).build();}
>> > > >
>> > > >
>> > > > And in simpler cases such as non-null checks:
>> > > > public Twiddle withUsername(String username) {
>> > > > checkNotNull(username, "username");
>> > > > checkArgument(!username.isEmpty(), "username can not be empty");
>> > > > ...
>> > > > }
>> > > >
>> > >
>> >
>>