You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Pranav Agarwal <pr...@gmail.com> on 2015/05/29 07:14:44 UTC
Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/
-----------------------------------------------------------
Review request for lens.
Bugs: LENS-590
https://issues.apache.org/jira/browse/LENS-590
Repository: lens
Description
-------
lens-590 Lens CLI "query results" command to support sync option
Diffs
-----
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
Diff: https://reviews.apache.org/r/34797/diff/
Testing
-------
tests didn't come out clean yet. Its intermittent and doesn't look to be related to the txn..
I'm running the tests, and in the meanwhile please provide feedback.
Thanks,
Pranav Agarwal
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review85952
-----------------------------------------------------------
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java
<https://reviews.apache.org/r/34797/#comment137777>
Let's keep the `help` of `CliOption` consise, as it goes in `cli.apt` as the format of command. Just `<async>` should do. And provide the description of the new arguemnt in help of `CliCommand`. This is a pattern followed for other commands too.
The main aim of `help` options is to have a good user documentation.
You can run `mvn site:run` and look at `localhost:8080/user/cli.html` to see how the help for the whole command looks.
- Rajat Khandelwal
On June 1, 2015, 1 p.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 1 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590
>
>
> per review comments from rajat: added cli.apt and adjusted cli params for query results commands
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
> src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests have come out clean
>
>
> Thanks,
>
> Pranav Agarwal
>
>
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review86394
-----------------------------------------------------------
Ship it!
Ship It!
- Rajat Khandelwal
On June 1, 2015, 9:29 p.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 9:29 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590
>
>
> per review comments from rajat: added cli.apt and adjusted cli params for query results commands
>
>
> improved help message for query results CLI option
>
>
> added a testcase
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
> src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests have come out clean
>
>
> Thanks,
>
> Pranav Agarwal
>
>
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Pranav Agarwal <pr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/
-----------------------------------------------------------
(Updated June 1, 2015, 3:59 p.m.)
Review request for lens.
Bugs: LENS-590
https://issues.apache.org/jira/browse/LENS-590
Repository: lens
Description (updated)
-------
lens-590
per review comments from rajat: added cli.apt and adjusted cli params for query results commands
improved help message for query results CLI option
added a testcase
Diffs (updated)
-----
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
Diff: https://reviews.apache.org/r/34797/diff/
Testing
-------
tests have come out clean
Thanks,
Pranav Agarwal
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review85962
-----------------------------------------------------------
lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java
<https://reviews.apache.org/r/34797/#comment137809>
Can we have one test case of `false` too?
- Rajat Khandelwal
On June 1, 2015, 2:45 p.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 2:45 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590
>
>
> per review comments from rajat: added cli.apt and adjusted cli params for query results commands
>
>
> improved help message for query results CLI option
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
> src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests have come out clean
>
>
> Thanks,
>
> Pranav Agarwal
>
>
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Pranav Agarwal <pr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/
-----------------------------------------------------------
(Updated June 1, 2015, 9:15 a.m.)
Review request for lens.
Bugs: LENS-590
https://issues.apache.org/jira/browse/LENS-590
Repository: lens
Description (updated)
-------
lens-590
per review comments from rajat: added cli.apt and adjusted cli params for query results commands
improved help message for query results CLI option
Diffs (updated)
-----
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
Diff: https://reviews.apache.org/r/34797/diff/
Testing
-------
tests have come out clean
Thanks,
Pranav Agarwal
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Pranav Agarwal <pr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/
-----------------------------------------------------------
(Updated June 1, 2015, 7:30 a.m.)
Review request for lens.
Bugs: LENS-590
https://issues.apache.org/jira/browse/LENS-590
Repository: lens
Description (updated)
-------
lens-590
per review comments from rajat: added cli.apt and adjusted cli params for query results commands
Diffs (updated)
-----
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
Diff: https://reviews.apache.org/r/34797/diff/
Testing (updated)
-------
tests have come out clean
Thanks,
Pranav Agarwal
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review85927
-----------------------------------------------------------
Ship it!
Changes look fine. Ship it once tests pass.
- Amareshwari Sriramadasu
On May 29, 2015, 5:14 a.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 5:14 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590 Lens CLI "query results" command to support sync option
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests didn't come out clean yet. Its intermittent and doesn't look to be related to the txn..
> I'm running the tests, and in the meanwhile please provide feedback.
>
>
> Thanks,
>
> Pranav Agarwal
>
>
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Pranav Agarwal <pr...@gmail.com>.
> On June 1, 2015, 6:18 a.m., Rajat Khandelwal wrote:
> > lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java, line 274
> > <https://reviews.apache.org/r/34797/diff/1/?file=974118#file974118line274>
> >
> > Please have a look at http://docs.spring.io/spring-shell/docs/current/api/org/springframework/shell/core/annotation/CliOption.html and decide what should be appropriate values for `specifiedDefaultValue` and `unspecifiedDefaultValue`
I think having specifiedDefaultValue as false is non-intutive. I'll set both unspecifiedDefaultValue and specifiedDefaultValue as true.
- Pranav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review85940
-----------------------------------------------------------
On June 1, 2015, 9:15 a.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 9:15 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590
>
>
> per review comments from rajat: added cli.apt and adjusted cli params for query results commands
>
>
> improved help message for query results CLI option
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
> src/site/apt/user/cli.apt 2016bfddf38751fd98bedb8163d61c6af5b99be3
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests have come out clean
>
>
> Thanks,
>
> Pranav Agarwal
>
>
Re: Review Request 34797: lens-590:Lens CLI "query results" command to
support sync option
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34797/#review85940
-----------------------------------------------------------
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java
<https://reviews.apache.org/r/34797/#comment137756>
Everytime you make changes in documentation/arguments in cli, please run `org.apache.lens.cli.doc.TestGenerateCLIUserDoc` once. It'll update the docs.
lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java
<https://reviews.apache.org/r/34797/#comment137757>
Please have a look at http://docs.spring.io/spring-shell/docs/current/api/org/springframework/shell/core/annotation/CliOption.html and decide what should be appropriate values for `specifiedDefaultValue` and `unspecifiedDefaultValue`
- Rajat Khandelwal
On May 29, 2015, 10:44 a.m., Pranav Agarwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34797/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 10:44 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-590
> https://issues.apache.org/jira/browse/LENS-590
>
>
> Repository: lens
>
>
> Description
> -------
>
> lens-590 Lens CLI "query results" command to support sync option
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/LensQueryCommands.java 6c4a1b5fc82a8fe4776919546d6f8c975ae0f514
> lens-cli/src/test/java/org/apache/lens/cli/TestLensQueryCommands.java 32a89ac98230d642738e1954b965c692c7235507
> lens-client/src/main/java/org/apache/lens/client/LensClient.java cd8bf166e11786ae8ec26df8d03dd381ef62d787
> lens-client/src/main/java/org/apache/lens/client/LensStatement.java 4472a80b9e8b8b7243ec6d15a854867bb24dcce1
>
> Diff: https://reviews.apache.org/r/34797/diff/
>
>
> Testing
> -------
>
> tests didn't come out clean yet. Its intermittent and doesn't look to be related to the txn..
> I'm running the tests, and in the meanwhile please provide feedback.
>
>
> Thanks,
>
> Pranav Agarwal
>
>