You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@lens.apache.org by Amareshwari Sriramadasu <am...@apache.org> on 2016/06/29 12:15:16 UTC

Review Request 49371: LENS-1168 : Add query launcher pool

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

Review request for lens.


Bugs: LENS-1168
    https://issues.apache.org/jira/browse/LENS-1168


Repository: lens


Description
-------

Changes include :
- Add query launcher pool which would launch queries after applying constraint checks.
- Add max concurrent launching queries constraint - to limit number parallel launches at any time.
- Update StatusPoller to skip launching queries
- Update cancelQuery to interrupt launcher.
- Added test to verify parallel launches.


Diffs
-----

  lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
  lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
  lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
  lens-server/src/main/resources/lensserver-default.xml 6dc322e 
  lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
  lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
  src/site/apt/admin/config.apt 5b76069 

Diff: https://reviews.apache.org/r/49371/diff/


Testing
-------


Thanks,

Amareshwari Sriramadasu

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 11, 2016, 6:12 a.m., Rajat Khandelwal wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java, line 205
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437565#file1437565line205>
> >
> >     I feel adding a new state would be better than adding a boolean which is true only for an intermediate phase between two particular states, and false every other time. Having the boolean makes understanding the stata machine difficult for new people. 
> >     
> >     Then state machine would be `NEW->QUEUED->LAUNCHING->LAUNCHED->RUNNING->...`

There are both pros and cons. Adding a new state can make the code obvious, but will touch all pieces of code.
Doing a state change would make it incompatible and end users have to understand new state more. And the use of LAUNCHING state is useful only for constraint checks, as of now.


- Amareshwari


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


On July 6, 2016, 8:47 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 8:47 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 11, 2016, 6:12 a.m., Rajat Khandelwal wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java, line 205
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437565#file1437565line205>
> >
> >     I feel adding a new state would be better than adding a boolean which is true only for an intermediate phase between two particular states, and false every other time. Having the boolean makes understanding the stata machine difficult for new people. 
> >     
> >     Then state machine would be `NEW->QUEUED->LAUNCHING->LAUNCHED->RUNNING->...`
> 
> Amareshwari Sriramadasu wrote:
>     There are both pros and cons. Adding a new state can make the code obvious, but will touch all pieces of code.
>     Doing a state change would make it incompatible and end users have to understand new state more. And the use of LAUNCHING state is useful only for constraint checks, as of now.

Dropping this. As LAUNCHING as seperate state will require separate handling upon restarts, which can be skipped when we have it as simple boolean.


- Amareshwari


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


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Rajat Khandelwal <ra...@gmail.com>.

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




lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java (line 205)
<https://reviews.apache.org/r/49371/#comment206986>

    I feel adding a new state would be better than adding a boolean which is true only for an intermediate phase between two particular states, and false every other time. Having the boolean makes understanding the stata machine difficult for new people. 
    
    Then state machine would be `NEW->QUEUED->LAUNCHING->LAUNCHED->RUNNING->...`



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1461)
<https://reviews.apache.org/r/49371/#comment206984>

    We'll need to stop the pool in `prepareStopping`


- Rajat Khandelwal


On July 6, 2016, 2:17 p.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 2:17 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 20, 2016, 8:53 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 1339
> > <https://reviews.apache.org/r/49371/diff/3/?file=1442092#file1442092line1339>
> >
> >     Do we need to await termination of this pool ?
> >     
> >     Cons : a query that takes lot of time to lauch will delay the shutdown.
> >     
> >     Pros : on startup we do not need to handle the case where we get a query in LAUNCHING state.

As we are have await for cancellationPool in prepare stopping, we can add launcher pool as well. Adding it here.


- Amareshwari


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


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.

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




lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1314)
<https://reviews.apache.org/r/49371/#comment208587>

    Do we need to await termination of this pool ?
    
    Cons : a query that takes lot of time to lauch will delay the shutdown.
    
    Pros : on startup we do not need to handle the case where we get a query in LAUNCHING state.


- Puneet Gupta


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 20, 2016, 8:36 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 773
> > <https://reviews.apache.org/r/49371/diff/3/?file=1442092#file1442092line773>
> >
> >     We might need to hanlde InterruptedException from Furture.cancel(true)

Future.cancel is not throwing any exception. So, I'm catching all exceptions here and checking if query is cancelled() or not.


> On July 20, 2016, 8:36 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 821
> > <https://reviews.apache.org/r/49371/diff/3/?file=1442092#file1442092line821>
> >
> >     Should we move this to QueryLauncher.run() finally block just in case launchQuery() deosn't get called.

makes sense, will update


- Amareshwari


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


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 20, 2016, 8:36 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 767
> > <https://reviews.apache.org/r/49371/diff/3/?file=1442092#file1442092line767>
> >
> >     Cancel code may require some synchronization.

Now that launchQuery is synchronized, this should not be required.


- Amareshwari


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


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.

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




lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 752)
<https://reviews.apache.org/r/49371/#comment208577>

    Cancel code may require some synchronization.



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 758)
<https://reviews.apache.org/r/49371/#comment208578>

    We might need to hanlde InterruptedException from Furture.cancel(true)



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 805)
<https://reviews.apache.org/r/49371/#comment208579>

    Should we move this to QueryLauncher.run() finally block just in case launchQuery() deosn't get called.



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1489)
<https://reviews.apache.org/r/49371/#comment208580>

    can we make this false ? Some threads will alaways be available to launch queries


- Puneet Gupta


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.

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


Ship it!




Ship It!

- Puneet Gupta


On July 22, 2016, 4:05 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 22, 2016, 4:05 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java be0e2c8 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 3ce8fc1 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   lens-server/src/test/resources/logback.xml a345054 
>   src/site/apt/admin/config.apt 852955b 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.

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

(Updated July 22, 2016, 4:05 a.m.)


Review request for lens.


Changes
-------

Test failure fixed


Bugs: LENS-1168
    https://issues.apache.org/jira/browse/LENS-1168


Repository: lens


Description
-------

Changes include :
- Add query launcher pool which would launch queries after applying constraint checks.
- Add max concurrent launching queries constraint - to limit number parallel launches at any time.
- Update StatusPoller to skip launching queries
- Update cancelQuery to interrupt launcher.
- Added test to verify parallel launches.


Diffs (updated)
-----

  lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
  lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
  lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
  lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
  lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java be0e2c8 
  lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
  lens-server/src/main/resources/lensserver-default.xml 3ce8fc1 
  lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
  lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
  lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
  lens-server/src/test/resources/logback.xml a345054 
  src/site/apt/admin/config.apt 852955b 

Diff: https://reviews.apache.org/r/49371/diff/


Testing
-------


Thanks,

Amareshwari Sriramadasu

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.

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

(Updated July 21, 2016, 11:56 a.m.)


Review request for lens.


Changes
-------

Review comments incorporated


Bugs: LENS-1168
    https://issues.apache.org/jira/browse/LENS-1168


Repository: lens


Description
-------

Changes include :
- Add query launcher pool which would launch queries after applying constraint checks.
- Add max concurrent launching queries constraint - to limit number parallel launches at any time.
- Update StatusPoller to skip launching queries
- Update cancelQuery to interrupt launcher.
- Added test to verify parallel launches.


Diffs (updated)
-----

  lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
  lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
  lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
  lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
  lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java be0e2c8 
  lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
  lens-server/src/main/resources/lensserver-default.xml 3ce8fc1 
  lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
  lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
  lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
  src/site/apt/admin/config.apt 852955b 

Diff: https://reviews.apache.org/r/49371/diff/


Testing
-------


Thanks,

Amareshwari Sriramadasu

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.

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

(Updated July 12, 2016, 5:41 a.m.)


Review request for lens.


Changes
-------

Incorporated review comments


Bugs: LENS-1168
    https://issues.apache.org/jira/browse/LENS-1168


Repository: lens


Description
-------

Changes include :
- Add query launcher pool which would launch queries after applying constraint checks.
- Add max concurrent launching queries constraint - to limit number parallel launches at any time.
- Update StatusPoller to skip launching queries
- Update cancelQuery to interrupt launcher.
- Added test to verify parallel launches.


Diffs (updated)
-----

  lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
  lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
  lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
  lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
  lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
  lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
  lens-server/src/main/resources/lensserver-default.xml 6dc322e 
  lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
  lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
  lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
  src/site/apt/admin/config.apt 5b76069 

Diff: https://reviews.apache.org/r/49371/diff/


Testing
-------


Thanks,

Amareshwari Sriramadasu

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, lines 787-793
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437569#file1437569line787>
> >
> >     Do we need to synchronize the entire run method on context ? 
> >     I can think of only one reason - cancel and launch happening parallely before this JIRA were synchronized. Not sure if there can be more reasons to do this.
> >     
> >     If we do syncronize , this code will not be needed.
> 
> Amareshwari Sriramadasu wrote:
>     Did not want to synchronize on the entire method, as other operations like getStatus or cancelQuery would block threads for this launching thread.

Doing synchronization on the whole launch - same as the earlier flow. cancel and updatestatus will wait if launch takes more time.


- Amareshwari


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


On July 12, 2016, 5:41 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 12, 2016, 5:41 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MetricsService.java 7fd2d81 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java d10ad09 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java 1e8d540 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java e77c7fa 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java d5e975a 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java 0df436a 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java, line 49
> > <https://reviews.apache.org/r/49371/diff/1-2/?file=1432918#file1432918line49>
> >
> >     shoudl we use driverLaunchedQueries.size() here.

Not changing as there are tests mocking it.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java, line 76
> > <https://reviews.apache.org/r/49371/diff/1-2/?file=1432919#file1432919line76>
> >
> >     Should we have a default value of 10/20 ?

Instead of 10/20, planning to default to max concurrent queries.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 2538
> > <https://reviews.apache.org/r/49371/diff/1-2/?file=1432921#file1432921line2538>
> >
> >     should we log the query id or will it come from MDC ?

Yes. Would come from MDC. Updated in all callers


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, lines 787-793
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437569#file1437569line787>
> >
> >     Do we need to synchronize the entire run method on context ? 
> >     I can think of only one reason - cancel and launch happening parallely before this JIRA were synchronized. Not sure if there can be more reasons to do this.
> >     
> >     If we do syncronize , this code will not be needed.

Did not want to synchronize on the entire method, as other operations like getStatus or cancelQuery would block threads for this launching thread.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 1484
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437569#file1437569line1484>
> >
> >     Do we need to have metrics around laucher pool(number of active thread, queries in launching state) ?

Not planning to add any such at the pool level. Thread dump would give details of all those. We can add gauge for queries being launched. Will try adding one.


> On July 8, 2016, 10:45 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 1493
> > <https://reviews.apache.org/r/49371/diff/2/?file=1437569#file1437569line1493>
> >
> >     do we need to make this class level varaible ?

No. The number here is mainly for thread number in launcher pool alone. Not across all pools.


- Amareshwari


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


On July 6, 2016, 8:47 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 8:47 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.

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




lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java (line 49)
<https://reviews.apache.org/r/49371/#comment206706>

    shoudl we use driverLaunchedQueries.size() here.



lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java (line 97)
<https://reviews.apache.org/r/49371/#comment206707>

    Suggestion : should we change the name to getIsLauchingCount()



lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java (line 76)
<https://reviews.apache.org/r/49371/#comment206708>

    Should we have a default value of 10/20 ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 2535)
<https://reviews.apache.org/r/49371/#comment206709>

    should we log the query id or will it come from MDC ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 659)
<https://reviews.apache.org/r/49371/#comment206713>

    Should we remove errorCollection from constructor too ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 752)
<https://reviews.apache.org/r/49371/#comment206714>

    org.apache.lens.server.session.LensSessionImpl#acquire is synchronized . Should we look at reducing the scope of synchronization here or in another JIRA ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (lines 773 - 779)
<https://reviews.apache.org/r/49371/#comment206717>

    Do we need to synchronize the entire run method on context ? 
    I can think of only one reason - cancel and launch happening parallely before this JIRA were synchronized. Not sure if there can be more reasons to do this.
    
    If we do syncronize , this code will not be needed.



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 801)
<https://reviews.apache.org/r/49371/#comment206718>

    should we do this in finally{}



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1461)
<https://reviews.apache.org/r/49371/#comment206720>

    Do we need to have metrics around laucher pool(number of active thread, queries in launching state) ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1470)
<https://reviews.apache.org/r/49371/#comment206719>

    do we need to make this class level varaible ?


- Puneet Gupta


On July 6, 2016, 8:47 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 8:47 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.

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

(Updated July 6, 2016, 8:47 a.m.)


Review request for lens.


Changes
-------

Incorporated review comments.


Bugs: LENS-1168
    https://issues.apache.org/jira/browse/LENS-1168


Repository: lens


Description
-------

Changes include :
- Add query launcher pool which would launch queries after applying constraint checks.
- Add max concurrent launching queries constraint - to limit number parallel launches at any time.
- Update StatusPoller to skip launching queries
- Update cancelQuery to interrupt launcher.
- Added test to verify parallel launches.


Diffs (updated)
-----

  lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
  lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
  lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
  lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
  lens-server/src/main/resources/lensserver-default.xml 6dc322e 
  lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
  lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
  src/site/apt/admin/config.apt 5b76069 

Diff: https://reviews.apache.org/r/49371/diff/


Testing
-------


Thanks,

Amareshwari Sriramadasu

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, lines 773-780
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432921#file1432921line773>
> >
> >     Do we need to hanlde InterrupedException separately and check if that was due to a cancel on this query ?

I feel its not required once we move it to Future cancellation. What do you say?


- Amareshwari


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


On July 6, 2016, 8:47 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 8:47 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, lines 773-780
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432921#file1432921line773>
> >
> >     Do we need to hanlde InterrupedException separately and check if that was due to a cancel on this query ?
> 
> Amareshwari Sriramadasu wrote:
>     I feel its not required once we move it to Future cancellation. What do you say?

Future.calcel() might cause InterruptedException. In that case it might be required.


- Puneet


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


On July 6, 2016, 8:47 a.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 8:47 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java, line 939
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432916#file1432916line939>
> >
> >     Should we increase number this since we are using a SynchronousQueue for laucherPool

Increasing to 100.


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java, line 40
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432918#file1432918line40>
> >
> >     should we call this maxConcurrentLaunches  and driver.max.concurrent.launches

Updated


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java, line 47
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432918#file1432918line47>
> >
> >     Should we cache launchedQueries.getQueries(selectedDriver) locally and use it for condition 1 and 2

Updated


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java, lines 36-42
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432919#file1432919line36>
> >
> >     driver.max.concurrent.launched.queries and 
> >     driver.max.concurrent.launches  seem similar. 
> >     
> >     Not sure how we should name them. Should we rename "driver.max.concurrent.launched.queries" to "driver.max.concurrent.running.queries" but this will be an incompatible change.

Not planning to do this.


> On July 6, 2016, 5:29 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, lines 661-665
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432921#file1432921line661>
> >
> >     errorCOllection is not required ?

yes. Not required.


- Amareshwari


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


On June 29, 2016, 12:15 p.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 12:15 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Puneet Gupta <pu...@gmail.com>.

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




lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java (line 939)
<https://reviews.apache.org/r/49371/#comment206328>

    Should we increase number this since we are using a SynchronousQueue for laucherPool



lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java (line 40)
<https://reviews.apache.org/r/49371/#comment206325>

    should we call this maxConcurrentLaunches  and driver.max.concurrent.launches



lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java (line 47)
<https://reviews.apache.org/r/49371/#comment206301>

    Should we cache launchedQueries.getQueries(selectedDriver) locally and use it for condition 1 and 2



lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java (lines 35 - 41)
<https://reviews.apache.org/r/49371/#comment206327>

    driver.max.concurrent.launched.queries and 
    driver.max.concurrent.launches  seem similar. 
    
    Not sure how we should name them. Should we rename "driver.max.concurrent.launched.queries" to "driver.max.concurrent.running.queries" but this will be an incompatible change.



lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java (line 48)
<https://reviews.apache.org/r/49371/#comment206305>

    Duplicate



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (lines 659 - 662)
<https://reviews.apache.org/r/49371/#comment206309>

    errorCOllection is not required ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 707)
<https://reviews.apache.org/r/49371/#comment206314>

    Should we store the Future object in QuereyContext instead and use it to cancel the task via Future.cancel(true)? Future#isCancelled can be logged after this call



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (lines 759 - 766)
<https://reviews.apache.org/r/49371/#comment206312>

    Do we need to hanlde InterrupedException separately and check if that was due to a cancel on this query ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 1487)
<https://reviews.apache.org/r/49371/#comment206310>

    should we disable this and let the core pool be available always for faster launching.


- Puneet Gupta


On June 29, 2016, 12:15 p.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 12:15 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Amareshwari Sriramadasu <am...@apache.org>.


> On July 6, 2016, 4:20 a.m., Raju Bairishetti wrote:
> > lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java, line 48
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432920#file1432920line48>
> >
> >     Can we change this to driver.max.concurrent.launchers?

Thanks for catching. Updating.


> On July 6, 2016, 4:20 a.m., Raju Bairishetti wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 712
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432921#file1432921line712>
> >
> >     We are already setting this in the queryLauncher thread. Seems redundant.

Removing it from queryLauncher thread. It will be required here, as launching constraint checker would check for this flag to know if it is launching.


> On July 6, 2016, 4:20 a.m., Raju Bairishetti wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java, line 713
> > <https://reviews.apache.org/r/49371/diff/1/?file=1432921#file1432921line713>
> >
> >     I feel this should be done by launcher thread only.
> >        I do not have much context about query constraints stuff. Just posting some thoughts here to understand little bit more about query constraints.
> >        
> >         Seems like adding here may violates the launcher queries constraints. Some times queries won't be launched even though none of the queries are in laucnhed state because of launchedQueries contains the queries which did not launch yet.
> >     
> >       *My Assumptions*: If all threads are busy in executing the queries and meanwhile mutliple queries are submitted then it launcher pool may not launch any of the queries after finishing the existing queries as it will not meet max launched queries constraint.

This has to be done before launching, for the constraint checker to check how many parallel launches are happening. This is to have limit on the parallel launches.

>> Seems like adding here may violates the launcher queries constraints. Some times queries won't be launched even though none of the queries are in laucnhed state because of launchedQueries contains the queries which did not launch yet.

No. QueryLauncher is not checking for any constraints, QuerySubmitter does. Once QuerySubmitter decides it can be launched, we can add them for parallel launch constraint to work.

>> My Assumptions: If all threads are busy in executing the queries and meanwhile mutliple queries are submitted then it launcher pool may not launch any of the queries after finishing the existing queries as it will not meet max launched queries constraint.

All threads will only do launch (asynchronously) and come out. They are not living around while executing.


- Amareshwari


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


On June 29, 2016, 12:15 p.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 12:15 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>

Re: Review Request 49371: LENS-1168 : Add query launcher pool

Posted by Raju Bairishetti <ra...@gmail.com>.

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




lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java (line 48)
<https://reviews.apache.org/r/49371/#comment206248>

    Can we change this to driver.max.concurrent.launchers?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 705)
<https://reviews.apache.org/r/49371/#comment206293>

    We are already setting this in the queryLauncher thread. Seems redundant.



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java (line 706)
<https://reviews.apache.org/r/49371/#comment206294>

    I feel this should be done by launcher thread only.
       I do not have much context about query constraints stuff. Just posting some thoughts here to understand little bit more about query constraints.
       
        Seems like adding here may violates the launcher queries constraints. Some times queries won't be launched even though none of the queries are in laucnhed state because of launchedQueries contains the queries which did not launch yet.
    
      *My Assumptions*: If all threads are busy in executing the queries and meanwhile mutliple queries are submitted then it launcher pool may not launch any of the queries after finishing the existing queries as it will not meet max launched queries constraint.


- Raju Bairishetti


On June 29, 2016, 12:15 p.m., Amareshwari Sriramadasu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49371/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 12:15 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1168
>     https://issues.apache.org/jira/browse/LENS-1168
> 
> 
> Repository: lens
> 
> 
> Description
> -------
> 
> Changes include :
> - Add query launcher pool which would launch queries after applying constraint checks.
> - Add max concurrent launching queries constraint - to limit number parallel launches at any time.
> - Update StatusPoller to skip launching queries
> - Update cancelQuery to interrupt launcher.
> - Added test to verify parallel launches.
> 
> 
> Diffs
> -----
> 
>   lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java bd9b1ab 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java 8ba0689 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraint.java b2319a9 
>   lens-server-api/src/main/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintFactory.java 442cd99 
>   lens-server-api/src/test/java/org/apache/lens/server/api/query/constraint/MaxConcurrentDriverQueriesConstraintTest.java 38b74ae 
>   lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java 2de098d 
>   lens-server/src/main/resources/lensserver-default.xml 6dc322e 
>   lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 0b38517 
>   lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 6af4225 
>   lens-server/src/test/java/org/apache/lens/server/query/constraint/ThreadSafeEstimatedQueryCollectionTest.java c8ebd0c 
>   src/site/apt/admin/config.apt 5b76069 
> 
> Diff: https://reviews.apache.org/r/49371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amareshwari Sriramadasu
> 
>