You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jared Stewart <js...@pivotal.io> on 2017/08/30 22:13:45 UTC

Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/
-----------------------------------------------------------

Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
-------

GEODE-3539: Add tests for List Members and Describe Member


Diffs
-----

  geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
  geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java bbb59d0755ffd2cf405f78c89b420a5279844e29 
  geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788f90ef68dc8ac338a4619646b4f3608293 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc404d33b48e5384348241c17ccf924f4627c 
  geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be16338ab9894878660054b75ff9cc6c3ec 


Diff: https://reviews.apache.org/r/62002/diff/1/


Testing
-------

Precheckin running


Thanks,

Jared Stewart

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jared Stewart <js...@pivotal.io>.

> On Aug. 31, 2017, 3:25 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/62002/diff/1/?file=1808175#file1808175line632>
> >
> >     See Gfsh.handleExecutionResult....
> >     
> >     any chance this could be called before gfsh gets the result for display? If so, then gfsh will display nothing, since hasNextLine() will return false from now on. Maybe for now just add a comment there saying only call this after gfsh displays the result already. The whole gfsh result displayer is a mess for now.
> 
> Jared Stewart wrote:
>     I only intended to use this method from tests and I think in that case there should always be a result.  I thought about adding it to some sort of helper/utility class (currently it resides in CliCommandTestBase), but it seems like it makes sense for it to live in CommandResult.  I'll add a comment with some explanation of how it should be used.
> 
> Jinmei Liao wrote:
>     Currently, without this function, I get the command result string using gfshRule.getoutputString method

Oh, I hadn't noticed that method.  I will use that for now instead.


- Jared


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
-----------------------------------------------------------


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc404d33b48e5384348241c17ccf924f4627c 
>   geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> -------
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jared Stewart <js...@pivotal.io>.

> On Aug. 31, 2017, 3:25 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/62002/diff/1/?file=1808175#file1808175line632>
> >
> >     See Gfsh.handleExecutionResult....
> >     
> >     any chance this could be called before gfsh gets the result for display? If so, then gfsh will display nothing, since hasNextLine() will return false from now on. Maybe for now just add a comment there saying only call this after gfsh displays the result already. The whole gfsh result displayer is a mess for now.

I only intended to use this method from tests and I think in that case there should always be a result.  I thought about adding it to some sort of helper/utility class (currently it resides in CliCommandTestBase), but it seems like it makes sense for it to live in CommandResult.  I'll add a comment with some explanation of how it should be used.


- Jared


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
-----------------------------------------------------------


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc404d33b48e5384348241c17ccf924f4627c 
>   geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> -------
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jinmei Liao <ji...@pivotal.io>.

> On Aug. 31, 2017, 3:25 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
> > Lines 632 (patched)
> > <https://reviews.apache.org/r/62002/diff/1/?file=1808175#file1808175line632>
> >
> >     See Gfsh.handleExecutionResult....
> >     
> >     any chance this could be called before gfsh gets the result for display? If so, then gfsh will display nothing, since hasNextLine() will return false from now on. Maybe for now just add a comment there saying only call this after gfsh displays the result already. The whole gfsh result displayer is a mess for now.
> 
> Jared Stewart wrote:
>     I only intended to use this method from tests and I think in that case there should always be a result.  I thought about adding it to some sort of helper/utility class (currently it resides in CliCommandTestBase), but it seems like it makes sense for it to live in CommandResult.  I'll add a comment with some explanation of how it should be used.

Currently, without this function, I get the command result string using gfshRule.getoutputString method


- Jinmei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
-----------------------------------------------------------


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc404d33b48e5384348241c17ccf924f4627c 
>   geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> -------
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184275
-----------------------------------------------------------




geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java
Lines 632 (patched)
<https://reviews.apache.org/r/62002/#comment260405>

    See Gfsh.handleExecutionResult....
    
    any chance this could be called before gfsh gets the result for display? If so, then gfsh will display nothing, since hasNextLine() will return false from now on. Maybe for now just add a comment there saying only call this after gfsh displays the result already. The whole gfsh result displayer is a mess for now.


- Jinmei Liao


On Aug. 30, 2017, 10:13 p.m., Jared Stewart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2017, 10:13 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69ebdd2ce5ffbab486fcb7a4dda71935586 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/CommandResult.java bbb59d0755ffd2cf405f78c89b420a5279844e29 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788f90ef68dc8ac338a4619646b4f3608293 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc404d33b48e5384348241c17ccf924f4627c 
>   geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be16338ab9894878660054b75ff9cc6c3ec 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/1/
> 
> 
> Testing
> -------
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/#review184289
-----------------------------------------------------------


Ship it!




Ship It!

- Jinmei Liao


On Aug. 31, 2017, 4:20 p.m., Jared Stewart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62002/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2017, 4:20 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-3539: Add tests for List Members and Describe Member
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc40 
>   geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be 
> 
> 
> Diff: https://reviews.apache.org/r/62002/diff/2/
> 
> 
> Testing
> -------
> 
> Precheckin running
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 62002: GEODE-3539: Add tests for List Members and Describe Member

Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62002/
-----------------------------------------------------------

(Updated Aug. 31, 2017, 4:20 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
-------

GEODE-3539: Add tests for List Members and Describe Member


Diffs (updated)
-----

  geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListMemberCommand.java ea88c69 
  geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MemberCommandsController.java ba5c788 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DescribeMembersCommandDUnitTest.java PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListMembersCommandDUnitTest.java PRE-CREATION 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java fe6bc40 
  geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java 17719be 


Diff: https://reviews.apache.org/r/62002/diff/2/

Changes: https://reviews.apache.org/r/62002/diff/1-2/


Testing
-------

Precheckin running


Thanks,

Jared Stewart