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
<[email protected]> 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 <[email protected]>
> 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 <[email protected]>
>> 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 <[email protected]
>> >
>> > 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 <
>> > > [email protected]> 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");
>> > > > ...
>> > > > }
>> > > >
>> > >
>> >
>>
