Re: Beam Slack channel

[Guillermo Rodríguez Cano]

 Thanks... and joined already! :)

/G.


On Fri, Jul 28, 2017 at 12:50 PM, James  wrote:

> Invite Sent.
>
> Guillermo Rodríguez Cano 于2017年7月28日周五 下午6:42写道：
>
> >   Hello,
> >
> >  as the subject hints... new here. Can someone add me to the Slack
> channel,
> > please?
> >
> > /G.
> >
>

Beam Slack channel

[Guillermo Rodríguez Cano]

  Hello,

 as the subject hints... new here. Can someone add me to the Slack channel,
please?

/G.

Re: Beam Slack channel

[James]

Invite Sent.

Guillermo Rodríguez Cano 于2017年7月28日周五 下午6:42写道：

>   Hello,
>
>  as the subject hints... new here. Can someone add me to the Slack channel,
> please?
>
> /G.
>

Build failed in Jenkins: beam_Release_NightlySnapshot #490

[Apache Jenkins Server]

See 


Changes:

[iemejia] [BEAM-1984] Fix scope for dependencies needed only for test purposes

[kirpichov] Fixes BigQueryIO.Write javadoc for to(ValueInSingleWindow)

[kirpichov] [BEAM-2640] Introduces Create.ofProvider(ValueProvider)

[kirpichov] [BEAM-2641] Introduces TextIO.read().withHintMatchesManyFiles()

[kirpichov] Adds AvroIO.read().withHintMatchesManyFiles()

--
[...truncated 381.31 KB...]
2017-07-28T07:03:45.232 [INFO] 
2017-07-28T07:03:45.232 [INFO] --- maven-deploy-plugin:2.8.2:deploy 
(default-deploy) @ beam-sdks-java-parent ---
2017-07-28T07:03:45.234 [INFO] Downloading: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/maven-metadata.xml
2017-07-28T07:03:45.478 [INFO] Downloaded: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/maven-metadata.xml
 (615 B at 2.5 KB/sec)
2017-07-28T07:03:45.479 [INFO] Uploading: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/beam-sdks-java-parent-2.2.0-20170728.070345-20.pom
2017-07-28T07:03:45.849 [INFO] Uploaded: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/beam-sdks-java-parent-2.2.0-20170728.070345-20.pom
 (3 KB at 5.7 KB/sec)
2017-07-28T07:03:45.849 [INFO] Downloading: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/maven-metadata.xml
2017-07-28T07:03:46.085 [INFO] Downloaded: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/maven-metadata.xml
 (538 B at 2.2 KB/sec)
2017-07-28T07:03:46.086 [INFO] Uploading: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/maven-metadata.xml
2017-07-28T07:03:46.459 [INFO] Uploaded: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/2.2.0-SNAPSHOT/maven-metadata.xml
 (615 B at 1.6 KB/sec)
2017-07-28T07:03:46.459 [INFO] Uploading: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/maven-metadata.xml
2017-07-28T07:03:46.828 [INFO] Uploaded: 
https://repository.apache.org/content/repositories/snapshots/org/apache/beam/beam-sdks-java-parent/maven-metadata.xml
 (538 B at 1.4 KB/sec)
[JENKINS] Archiving disabled
2017-07-28T07:03:47.939 [INFO]  
   
2017-07-28T07:03:47.939 [INFO] 

2017-07-28T07:03:47.939 [INFO] Building Apache Beam :: SDKs :: Java :: Core 
2.2.0-SNAPSHOT
2017-07-28T07:03:47.939 [INFO] 

2017-07-28T07:03:47.940 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/jacoco/jacoco-maven-plugin/0.7.8/jacoco-maven-plugin-0.7.8.pom
[INFO] I/O exception (java.net.SocketException) caught when processing request 
to {s}->https://repo.maven.apache.org:443: Connection reset
[INFO] Retrying request to {s}->https://repo.maven.apache.org:443
[INFO] I/O exception (java.net.SocketException) caught when processing request 
to {s}->https://repo.maven.apache.org:443: Connection reset
[INFO] Retrying request to {s}->https://repo.maven.apache.org:443
[INFO] I/O exception (java.net.SocketException) caught when processing request 
to {s}->https://repo.maven.apache.org:443: Connection reset
[INFO] Retrying request to {s}->https://repo.maven.apache.org:443
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] Archiving disabled
[JENKINS] 

Re: Style of messages for checkArgument/checkNotNull in IOs

[Raghu Angadi]

On Fri, Jul 28, 2017 at 11:21 AM, Thomas Groh 
wrote:

> I'm in favor of the wording in the style of the first: it's an immediately
> actionable message that will have an associated stack trace, but will
> provide the parameter in plaintext so the caller doesn't have to dig
> through the invoked code, they can just look at the documentation.
>
> I've recently been convinced that all input validation should go through
> `checkArgument` (including for nulls) rather than 'checkNotNull', due to
> the type of exception thrown, so I'd usually prefer using that as the
> `Preconditions` method. Beyond that, +1
>

+1. For me, main requirement is that it should be aimed at end user and
should be actionable without having to look at the code. It is always a
delight to receive message useful error as early as possible. This also
applies when checks are made in validate().


>
> On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
> kirpic...@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

[Kenneth Knowles]

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 <
kirpic...@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

[Jean-Baptiste Onofré]

Hi

I'm a bit concerned because it has been discussed in a Jira and we discussed 
especially about this with Kenn and I. It dates from the incubation period.

I changes most of the IOs I did for style 2 whereas I started with style 1.

I'm ready to change again on the IOs but we agreed that checkArgument and 
checkState are better to explain cause to the users more than checkNotNull.

For me it's important to keep a coherence in the project.
Either way is fine as far as it's consistent and it's not a single person 
decision.

Regards
JB


On Jul 28, 2017, 20:18, at 20:18, Eugene Kirpichov 
 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

[Kenneth Knowles]

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 
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 
> 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 <
> > kirpic...@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

[Jean-Baptiste Onofré]

Agree

Regards
JB

On Jul 28, 2017, 20:22, at 20:22, Thomas Groh  wrote:
>I'm in favor of the wording in the style of the first: it's an
>immediately
>actionable message that will have an associated stack trace, but will
>provide the parameter in plaintext so the caller doesn't have to dig
>through the invoked code, they can just look at the documentation.
>
>I've recently been convinced that all input validation should go
>through
>`checkArgument` (including for nulls) rather than 'checkNotNull', due
>to
>the type of exception thrown, so I'd usually prefer using that as the
>`Preconditions` method. Beyond that, +1
>
>On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
>kirpic...@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");
>>   ...
>> }
>>

Style of messages for checkArgument/checkNotNull in IOs

[Eugene Kirpichov]

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

[Thomas Groh]

I'm in favor of the wording in the style of the first: it's an immediately
actionable message that will have an associated stack trace, but will
provide the parameter in plaintext so the caller doesn't have to dig
through the invoked code, they can just look at the documentation.

I've recently been convinced that all input validation should go through
`checkArgument` (including for nulls) rather than 'checkNotNull', due to
the type of exception thrown, so I'd usually prefer using that as the
`Preconditions` method. Beyond that, +1

On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
kirpic...@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

[Eugene Kirpichov]

Okay, so then let's change guidance to:
- Always use checkArgument to check stuff that the user supplied
- Use style 1 for messages on the checkArgument
?

On Fri, Jul 28, 2017 at 11:31 AM Jean-Baptiste Onofré 
wrote:

> Agree
>
> Regards
> JB
>
> On Jul 28, 2017, 20:22, at 20:22, Thomas Groh 
> wrote:
> >I'm in favor of the wording in the style of the first: it's an
> >immediately
> >actionable message that will have an associated stack trace, but will
> >provide the parameter in plaintext so the caller doesn't have to dig
> >through the invoked code, they can just look at the documentation.
> >
> >I've recently been convinced that all input validation should go
> >through
> >`checkArgument` (including for nulls) rather than 'checkNotNull', due
> >to
> >the type of exception thrown, so I'd usually prefer using that as the
> >`Preconditions` method. Beyond that, +1
> >
> >On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
> >kirpic...@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

[Eugene Kirpichov]

(I don't think changing messages in existing IOs is a high priority, btw -
just a nice-to-have, as long as we have guidance for future IOs)

On Fri, Jul 28, 2017 at 11:37 AM Eugene Kirpichov 
wrote:

> Okay, so then let's change guidance to:
> - Always use checkArgument to check stuff that the user supplied
> - Use style 1 for messages on the checkArgument
> ?
>
> On Fri, Jul 28, 2017 at 11:31 AM Jean-Baptiste Onofré 
> wrote:
>
>> Agree
>>
>> Regards
>> JB
>>
>> On Jul 28, 2017, 20:22, at 20:22, Thomas Groh 
>> wrote:
>> >I'm in favor of the wording in the style of the first: it's an
>> >immediately
>> >actionable message that will have an associated stack trace, but will
>> >provide the parameter in plaintext so the caller doesn't have to dig
>> >through the invoked code, they can just look at the documentation.
>> >
>> >I've recently been convinced that all input validation should go
>> >through
>> >`checkArgument` (including for nulls) rather than 'checkNotNull', due
>> >to
>> >the type of exception thrown, so I'd usually prefer using that as the
>> >`Preconditions` method. Beyond that, +1
>> >
>> >On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov <
>> >kirpic...@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

[Reuven Lax]

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 
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 <
> kirpic...@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: Dynamic file-based sinks

[Reuven Lax]

The AvroIO PR is now merged, so you can write to different destinations
based on the value. It's available in head, and will be in Beam 2.2.0.

On Wed, Jul 26, 2017 at 10:00 AM, Reuven Lax  wrote:

> Yes, there was! TextIO support is already merged into Beam (it missed the
> 2.1 cutoff, so it will be in Beam 2.2.0). AvroIO support is in
> https://github.com/apache/beam/pull/3541. This is almost ready to merge -
> still waiting for final review from kennknowles on the Beam translation
> changes.
>
> Nobody is working on BigtableIO yet, however the framework used in
> BigQueryIO, TextIO, and AvroIO should be easy to generalize to other sinks.
>
> On Wed, Jul 26, 2017 at 7:41 AM, Josh  wrote:
>
>> Hi all,
>>
>> Was there any progress on this recently? I am particularly interested in
>> using value-dependent destinations in BigtableIO (writing to a specific
>> table depending on the value) and AvroIO (writing to specific GCS buckets
>> depending on the value).
>>
>> Thanks,
>> Josh
>>
>> On Fri, Jun 9, 2017 at 5:35 PM, Reuven Lax 
>> wrote:
>>
>> > I'm putting together a proof-of-concept PR for option 1 to see how it
>> > looks.
>> >
>> > On Thu, Jun 8, 2017 at 4:07 PM, Reuven Lax  wrote:
>> >
>> > > After looking at everyone's comments, I think option 1 is the better
>> > > approach - map destinations to a FilenamePolicy. It is a good
>> parallel to
>> > > what we do in BigQueryIO (the main difference is that we're mapping
>> to a
>> > > sharded filename, instead of a single destination like in BigQueryIO).
>> > >
>> > > The main limitation is that numShards cannot be dynamic per
>> destination.
>> > I
>> > > think that's fine for two reasons:
>> > >
>> > > 1. We generally discourage people from statically setting numShards,
>> as
>> > > often runner-determined sharding is better.
>> > > 2. In a case where users know that certain types of output files need
>> a
>> > > different number of shards, they can always partition. e.g. partition
>> > into
>> > > a 10-shard and a 100-shard sink, with each sink writing dynamic files.
>> > >
>> > > Eugene also brought up destination directory, but that part of the
>> > > FilenamePolicy interface is more a hint than anything else.
>> > > DestinationDirectory is realistically just the base directory for the
>> > temp
>> > > files, and the FilenamePolicy is free to ignore it.
>> > >
>> > > Reuven
>> > >
>> > > On Wed, May 24, 2017 at 1:54 PM, Eugene Kirpichov <
>> > > kirpic...@google.com.invalid> wrote:
>> > >
>> > >> Hmm, on one hand this looks syntactically very appealing, on the
>> other
>> > >> hand, it's icky to have a function return a PTransform at runtime,
>> only
>> > to
>> > >> have some information be immediately extracted from that transform.
>> > >> Moreover, not all TextIO.Write transforms will be legal to return -
>> e.g.
>> > >> most likely you're not allowed to return a transform that itself uses
>> > >> dynamic destinations.
>> > >>
>> > >> We should think more about how to decompose this problem.
>> > >> I think there are 2 natural elements to writing files:
>> > >> 1) where to put the files (let's call this file location)
>> > >> 2) how to write to a single file (let's call this file format. In
>> case
>> > of
>> > >> Avro, this may theoretically include e.g. schema to be embedded in
>> the
>> > >> file).
>> > >> There should be represented by different interfaces/classes in the
>> API.
>> > >>
>> > >> Then:
>> > >> - Writing a set of elements to a single file location using a single
>> > file
>> > >> format = "write operation"
>> > >> - WriteFiles is able to route different elements to different write
>> > >> operations, with potentially different both locations and formats.
>> I.e.
>> > >> it's configured by something like BQ's DynamicDestinations
>> > >> - TextIO and AvroIO are thin wrappers over WriteFiles
>> > >> - AvroIO in the future may be extended to support different schemas
>> for
>> > >> different files - then it would be even more like BigQuery: it'd take
>> > also
>> > >> a SerializableFunction and a
>> > >> SerializableFunction. That means that perhaps
>> it
>> > >> may
>> > >> provide its own DynamicDestinations-like API to its users, more
>> specific
>> > >> than the one exposed by low-level WriteFiles.
>> > >>
>> > >> This is pretty vague, but I think "AvroIO with dynamic schema and
>> with
>> > >> (type of input PCollection = T) != (type being written =
>> GenericRecord)"
>> > >> is
>> > >> a good target to guide search for the perfect API. WDYT?
>> > >>
>> > >> On Wed, May 24, 2017 at 11:24 AM Reuven Lax > >
>> > >> wrote:
>> > >>
>> > >> > Did you see that I modified the second proposal so that users can
>> map
>> > >> > DestinationT to the actual PTransform (i.e. DestinationT->TextIO or
>> > >> > DestinationT->AvroIO). This means that users do not have to deal
>> with
>> > >> >