Sure, I've updated the PR accordingly. On Tue, Jun 27, 2017 at 9:35 AM Lukasz Cwik <[email protected]> wrote:
> Since PipelineOptionsFactory knows whether fromArgs was used, I think you > can just store a boolean in the builder[1] and create a new method on > PipelineOptionsValidator to print the "CLI" version of the output. > > Also, I think you want the class+method for the non CLI approach since more > then one interface may define the same method and this tells the user > exactly where to look. > > 1: > > https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java#L182 > > On Mon, Jun 26, 2017 at 5:38 PM, Manu Zhang <[email protected]> > wrote: > > > Thanks Kenn and Lukasz. > > > > How about we add a hidden option to `PipelineOptions` and set depending > on > > `args == null` in `PipelinOptionsFactory` ? > > Then, we can get in `PipelineOptionsValidator#validate` and there is no > > need to change the interface. > > > > @Hidden > > boolean getFromCLI(); > > void setFromCLI(boolean fromCLI); > > > > > > Besides, for programmers, do you think we need to print `public abstract > > java.lang.String > > org.apache.beam.sdk.options.PipelineOptionsValidatorTest$ > > Required.getObject()` > > or just `getObject()` ? > > > > > > On Tue, Jun 27, 2017 at 6:24 AM Lukasz Cwik <[email protected]> > > wrote: > > > > > Sounds good to me, thus the change as currently proposed in PR/3438 > needs > > > some work to do this split. > > > > > > On Mon, Jun 26, 2017 at 3:13 PM, Kenneth Knowles > <[email protected] > > > > > > wrote: > > > > > > > I do think it is the responsibility of the CLI to own this; we > > definitely > > > > shouldn't rewrite PipelineOptions validation entirely, or > programmatic > > > uses > > > > will have needless noise or bad messages. I think that when someone > > calls > > > > fromArgs() they have sufficiently indicated that they are a CLI code > > path > > > > and we can do the right thing. > > > > > > > > Separate from that, it might be handy to expose such a utility for > > users > > > > that build their command line options via some other mechanism. > > > > > > > > On Mon, Jun 26, 2017 at 3:07 PM, Lukasz Cwik > <[email protected] > > > > > > > wrote: > > > > > > > > > In this case, we specifically asked in code to validate the > options: > > > > > https://github.com/apache/beam/blob/1ea1de4aa9d32e3c5a596ccd7d84af > > > > > f1cc2a7428/examples/java/src/main/java/org/apache/beam/ > > > > > examples/WordCount.java#L173 > > > > > > > > > > It seems like we could know whether PipelineOptions validation is > > > > happening > > > > > from the PipelineOptionsFactory.fromArgs() and/or have a special > CLI > > > > > option > > > > > like --validate (similar to --help) that would provide names based > > upon > > > > > arguments. > > > > > > > > > > In general, the PipelineOptionsValidator / .withValidation() method > > > > should > > > > > provide information about what method has not been set when not > used > > > > from a > > > > > CLI. > > > > > > > > > > Or we make sure both are listed to not have this dichotomy based > upon > > > how > > > > > PipelineOptions are used. > > > > > > > > > > On Mon, Jun 26, 2017 at 10:10 AM, Kenneth Knowles > > > <[email protected] > > > > > > > > > > wrote: > > > > > > > > > > > We should decouple CLI parsing validation from programmatic > > > > > PipelineOptions > > > > > > validation. > > > > > > > > > > > > On Mon, Jun 26, 2017 at 9:56 AM, Lukasz Cwik > > > <[email protected] > > > > > > > > > > > wrote: > > > > > > > > > > > > > This change assumes that all users are CLI users (compared to > the > > > > > > existing > > > > > > > code which assumed that all users were programmatic users). > > Should > > > we > > > > > > word > > > > > > > the message so its useful for both CLI and users who set the > > > options > > > > > > > programmatically? > > > > > > > > > > > > > > On Mon, Jun 26, 2017 at 6:00 AM, Jean-Baptiste Onofré < > > > > [email protected] > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Thanks Manu ! > > > > > > > > > > > > > > > > I will review it. > > > > > > > > > > > > > > > > Regards > > > > > > > > JB > > > > > > > > > > > > > > > > > > > > > > > > On 06/26/2017 02:59 PM, Manu Zhang wrote: > > > > > > > > > > > > > > > >> Thanks Kenn and JB, I just filed > > > > > > > >> https://issues.apache.org/jira/browse/BEAM-2514 and PR > > > > > > > >> https://github.com/apache/beam/pull/3438 > > > > > > > >> Please help to review. > > > > > > > >> > > > > > > > >> Thanks, > > > > > > > >> Manu > > > > > > > >> > > > > > > > >> On Mon, Jun 26, 2017 at 12:20 PM Jean-Baptiste Onofré < > > > > > > [email protected]> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >> Hi Manu, > > > > > > > >>> > > > > > > > >>> Agree, it makes sense. > > > > > > > >>> > > > > > > > >>> Regards > > > > > > > >>> JB > > > > > > > >>> > > > > > > > >>> On 06/25/2017 12:57 PM, Manu Zhang wrote: > > > > > > > >>> > > > > > > > >>>> Hi all, > > > > > > > >>>> > > > > > > > >>>> Currently, if a required option is missing for a Beam > > > pipeline, > > > > > the > > > > > > > >>>> error > > > > > > > >>>> message is like > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>>> Exception in thread "main" > > > java.lang.IllegalArgumentException: > > > > > > > Missing > > > > > > > >>>>> required value for [public abstract java.lang.String > > > > > > > >>>>> org.apache.beam.examples.WordCount$WordCountOptions. > > > > getOutput(), > > > > > > > "Path > > > > > > > >>>>> > > > > > > > >>>> of > > > > > > > >>> > > > > > > > >>>> the file to write to"]. > > > > > > > >>>>> > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>> This is quite long but doesn't give any hint to users > about > > > the > > > > > > > required > > > > > > > >>>> option. Instead, I'm thinking about something like > > > > > > > >>>> > > > > > > > >>>> Exception in thread "main" java.lang. > > IllegalArgumentException: > > > > > > > Missing > > > > > > > >>>> > > > > > > > >>>>> required option [--output, "Path of the file to write > to"]. > > > > > > > >>>>> > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>> It can be achieved by adding a method that *returns the > > option > > > > > from > > > > > > a > > > > > > > >>>> method* to ProxyInvocationHandler > > > > > > > >>>> < > > > > > > > >>>> > > > > > > > >>> https://github.com/apache/beam/blob/master/sdks/java/core/ > > > > > > > >>> src/main/java/org/apache/beam/sdk/options/ > > > > > > ProxyInvocationHandler.java > > > > > > > >>> > > > > > > > >>>> > > > > > > > >>>> class. > > > > > > > >>>> > > > > > > > >>>> public String getOption(Method method) { > > > > > > > >>>> return gettersToPropertyNames.get(method.getName()); > > > > > > > >>>> } > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>> This may look general for ProxyInvocationHandler but you > get > > > the > > > > > > idea. > > > > > > > >>>> > > > > > > > >>>> WDYT? > > > > > > > >>>> > > > > > > > >>>> Thanks, > > > > > > > >>>> Manu > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>> -- > > > > > > > >>> Jean-Baptiste Onofré > > > > > > > >>> [email protected] > > > > > > > >>> http://blog.nanthrax.net > > > > > > > >>> Talend - http://www.talend.com > > > > > > > >>> > > > > > > > >>> > > > > > > > >> > > > > > > > > -- > > > > > > > > Jean-Baptiste Onofré > > > > > > > > [email protected] > > > > > > > > http://blog.nanthrax.net > > > > > > > > Talend - http://www.talend.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
