You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Raju Bairishetti <ra...@gmail.com> on 2016/03/03 01:35:55 UTC
Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/
-----------------------------------------------------------
Review request for lens.
Bugs: lens-853
https://issues.apache.org/jira/browse/lens-853
Repository: lens
Description
-------
Setting isConnectionActive flag in setClient method instead of in getClient
closeConnection should close lensclient instead of singleton lens client
Diffs
-----
lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
Diff: https://reviews.apache.org/r/44305/diff/
Testing
-------
Thanks,
Raju Bairishetti
Re: Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
Posted by Rajat Khandelwal <ra...@gmail.com>.
> On March 3, 2016, 12:13 p.m., Rajat Khandelwal wrote:
> > lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java, line 92
> > <https://reviews.apache.org/r/44305/diff/1/?file=1278505#file1278505line92>
> >
> > I think this change is redundant.
> >
> > Shell has beans, instances of `LensQueryCommand, Lens...Command` etc. Each instance inherits from `BaseLensCommand`, which has a `lensClient` instance obtained from singleton wrapper. So all the beans have their own instance of `LensClient`, but the instance is same thanks to singleton wrapper.
> >
> > So at this line, `getClientWrapper().getClient()` should be same as `lensClient`.
> >
> > That explained, please look into `closeConnection` function since it will be called by all the beans on the same instance. Multiple calls to `closeConnection` should not be affecting the state of the CLI.
>
> Raju Bairishetti wrote:
> bq. So at this line, getClientWrapper().getClient() should be same as lensClient.
>
> Yes, lens client will be same as getClientWrapper().getClient() only for first LensConnection. If user creates mutliple connections (in test cases ) by creating lens client then closing only getClientWrapper().getClient() will close only one connection.
>
>
>
> Example:
>
> LensClient client = new LensClient();
> LensConnectionCommands commands1 = new LensConnectionCommands();
> commands.setClient(client);
> ...
>
> LensClient client = new LensClient();
> LensConnectionCommands commands2 = new LensConnectionCommands();
> commands.setClient(client);
> ...
>
> commands1.quitshell
> commands2.quitshell
>
>
> If I am not wrong, it will close only teardown only one connection instead of two connections
makes sense. +1
- Rajat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/#review121808
-----------------------------------------------------------
On March 3, 2016, 6:05 a.m., Raju Bairishetti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44305/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 6:05 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: lens-853
> https://issues.apache.org/jira/browse/lens-853
>
>
> Repository: lens
>
>
> Description
> -------
>
> Setting isConnectionActive flag in setClient method instead of in getClient
> closeConnection should close lensclient instead of singleton lens client
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
> lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
> lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
>
> Diff: https://reviews.apache.org/r/44305/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Raju Bairishetti
>
>
Re: Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
Posted by Raju Bairishetti <ra...@gmail.com>.
> On March 3, 2016, 6:43 a.m., Rajat Khandelwal wrote:
> > lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java, line 92
> > <https://reviews.apache.org/r/44305/diff/1/?file=1278505#file1278505line92>
> >
> > I think this change is redundant.
> >
> > Shell has beans, instances of `LensQueryCommand, Lens...Command` etc. Each instance inherits from `BaseLensCommand`, which has a `lensClient` instance obtained from singleton wrapper. So all the beans have their own instance of `LensClient`, but the instance is same thanks to singleton wrapper.
> >
> > So at this line, `getClientWrapper().getClient()` should be same as `lensClient`.
> >
> > That explained, please look into `closeConnection` function since it will be called by all the beans on the same instance. Multiple calls to `closeConnection` should not be affecting the state of the CLI.
bq. So at this line, getClientWrapper().getClient() should be same as lensClient.
Yes, lens client will be same as getClientWrapper().getClient() only for first LensConnection. If user creates mutliple connections (in test cases ) by creating lens client then closing only getClientWrapper().getClient() will close only one connection.
Example:
LensClient client = new LensClient();
LensConnectionCommands commands1 = new LensConnectionCommands();
commands.setClient(client);
...
LensClient client = new LensClient();
LensConnectionCommands commands2 = new LensConnectionCommands();
commands.setClient(client);
...
commands1.quitshell
commands2.quitshell
If I am not wrong, it will close only teardown only one connection instead of two connections
- Raju
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/#review121808
-----------------------------------------------------------
On March 3, 2016, 12:35 a.m., Raju Bairishetti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44305/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 12:35 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: lens-853
> https://issues.apache.org/jira/browse/lens-853
>
>
> Repository: lens
>
>
> Description
> -------
>
> Setting isConnectionActive flag in setClient method instead of in getClient
> closeConnection should close lensclient instead of singleton lens client
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
> lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
> lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
>
> Diff: https://reviews.apache.org/r/44305/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Raju Bairishetti
>
>
Re: Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/#review121808
-----------------------------------------------------------
lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java (line 92)
<https://reviews.apache.org/r/44305/#comment183644>
I think this change is redundant.
Shell has beans, instances of `LensQueryCommand, Lens...Command` etc. Each instance inherits from `BaseLensCommand`, which has a `lensClient` instance obtained from singleton wrapper. So all the beans have their own instance of `LensClient`, but the instance is same thanks to singleton wrapper.
So at this line, `getClientWrapper().getClient()` should be same as `lensClient`.
That explained, please look into `closeConnection` function since it will be called by all the beans on the same instance. Multiple calls to `closeConnection` should not be affecting the state of the CLI.
- Rajat Khandelwal
On March 3, 2016, 6:05 a.m., Raju Bairishetti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44305/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 6:05 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: lens-853
> https://issues.apache.org/jira/browse/lens-853
>
>
> Repository: lens
>
>
> Description
> -------
>
> Setting isConnectionActive flag in setClient method instead of in getClient
> closeConnection should close lensclient instead of singleton lens client
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
> lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
> lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
>
> Diff: https://reviews.apache.org/r/44305/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Raju Bairishetti
>
>
Re: Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/#review121815
-----------------------------------------------------------
Ship it!
Ship It!
- Rajat Khandelwal
On March 3, 2016, 6:05 a.m., Raju Bairishetti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44305/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 6:05 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: lens-853
> https://issues.apache.org/jira/browse/lens-853
>
>
> Repository: lens
>
>
> Description
> -------
>
> Setting isConnectionActive flag in setClient method instead of in getClient
> closeConnection should close lensclient instead of singleton lens client
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
> lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
> lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
>
> Diff: https://reviews.apache.org/r/44305/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Raju Bairishetti
>
>
Re: Review Request 44305: LENS-853: Quitting shell(quitshell()) is not
closing session in some cases
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44305/#review121794
-----------------------------------------------------------
Ship it!
Ship It!
- Amareshwari Sriramadasu
On March 3, 2016, 12:35 a.m., Raju Bairishetti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44305/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 12:35 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: lens-853
> https://issues.apache.org/jira/browse/lens-853
>
>
> Repository: lens
>
>
> Description
> -------
>
> Setting isConnectionActive flag in setClient method instead of in getClient
> closeConnection should close lensclient instead of singleton lens client
>
>
> Diffs
> -----
>
> lens-cli/src/main/java/org/apache/lens/cli/commands/BaseLensCommand.java 0c10198
> lens-cli/src/test/java/org/apache/lens/cli/TestLensConnectionCliCommands.java 558e97f
> lens-server/src/main/java/org/apache/lens/server/session/SessionResource.java ac77418
>
> Diff: https://reviews.apache.org/r/44305/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Raju Bairishetti
>
>