Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-24 Thread Amareshwari Sriramadasu 

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


Ship it!




Ship It!

- Amareshwari Sriramadasu


On April 24, 2017, 11:43 a.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 24, 2017, 11:43 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   lens-api/src/main/resources/lens-errors.conf 236d678 
>   
> lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java
>  14a31e2 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java
>  7e9133e 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-24 Thread Lavkesh Lahngir 

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

(Updated April 24, 2017, 11:43 a.m.)


Review request for lens.


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


Repository: lens


Description
---

1. Cancel estimates for other drivers when the estimate pool queue is full.
2. Return the appropriate error message


Diffs (updated)
-

  lens-api/src/main/resources/lens-errors.conf 236d678 
  
lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java 
14a31e2 
  
lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
 c6fbeda 
  
lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 
7e9133e 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
7d8c977 
  lens-server/src/test/resources/lens-site.xml 334a9c4 


Diff: https://reviews.apache.org/r/58604/diff/4/

Changes: https://reviews.apache.org/r/58604/diff/3-4/


Testing
---


Thanks,

Lavkesh Lahngir

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Lavkesh Lahngir 


> On April 21, 2017, 9:34 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
> > Lines 1716 (patched)
> > 
> >
> > Not sure if logging the driver name makes sense. Also Should we log 
> > this as error or warning is debatable ?

hmm. I guess logging does not hurt.. we can see which ones were rejected and 
which ones were cancelled.


> On April 21, 2017, 9:34 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
> > Lines 1722 (patched)
> > 
> >
> > can we use for(RewriteEstimateRunnable r : runnables) instead ?

uh. In this loop we are fetching from two lists.


- Lavkesh


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


On April 21, 2017, 12:10 p.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 21, 2017, 12:10 p.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   lens-api/src/main/resources/lens-errors.conf 236d678 
>   
> lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java
>  14a31e2 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java
>  7e9133e 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Lavkesh Lahngir 

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

(Updated April 21, 2017, 12:10 p.m.)


Review request for lens.


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


Repository: lens


Description
---

1. Cancel estimates for other drivers when the estimate pool queue is full.
2. Return the appropriate error message


Diffs (updated)
-

  lens-api/src/main/resources/lens-errors.conf 236d678 
  
lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java 
14a31e2 
  
lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
 c6fbeda 
  
lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 
7e9133e 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
7d8c977 
  lens-server/src/test/resources/lens-site.xml 334a9c4 


Diff: https://reviews.apache.org/r/58604/diff/3/

Changes: https://reviews.apache.org/r/58604/diff/2-3/


Testing
---


Thanks,

Lavkesh Lahngir

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Lavkesh Lahngir 


> On April 21, 2017, 11:49 a.m., Amareshwari Sriramadasu wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/error/LensCubeErrorCode.java
> > Lines 68 (patched)
> > 
> >
> > Should not be going into Cube errors, should be moved to 
> > LensServerErrorCode.
> > 
> > And accordingly change error code and position in lens-errors

sure.


> On April 21, 2017, 11:49 a.m., Amareshwari Sriramadasu wrote:
> > lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
> > Lines 1988 (patched)
> > 
> >
> > why is it only 10, when 20 threads are launched?

Typo.


> On April 21, 2017, 11:49 a.m., Amareshwari Sriramadasu wrote:
> > lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
> > Lines 1989 (patched)
> > 
> >
> > Should we be joining for all vs interrupt them and join? Interrupting 
> > would reduce test time.

Only few threads will go to FailQueryDriver. Interrupting might not simulate 
the rejection error.


> On April 21, 2017, 11:49 a.m., Amareshwari Sriramadasu wrote:
> > lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
> > Lines 1991 (patched)
> > 
> >
> > Add assert for howmany failed as well.

number of failures are not totally deterministic.


- Lavkesh


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


On April 21, 2017, 11:40 a.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 21, 2017, 11:40 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   lens-api/src/main/resources/lens-errors.conf 236d678 
>   lens-cube/src/main/java/org/apache/lens/cube/error/LensCubeErrorCode.java 
> babe3de 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java
>  7e9133e 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Amareshwari Sriramadasu 

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




lens-cube/src/main/java/org/apache/lens/cube/error/LensCubeErrorCode.java
Lines 68 (patched)


Should not be going into Cube errors, should be moved to 
LensServerErrorCode.

And accordingly change error code and position in lens-errors



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 1988 (patched)


why is it only 10, when 20 threads are launched?



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 1989 (patched)


Should we be joining for all vs interrupt them and join? Interrupting would 
reduce test time.



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 1991 (patched)


Add assert for howmany failed as well.


- Amareshwari Sriramadasu


On April 21, 2017, 11:40 a.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 21, 2017, 11:40 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   lens-api/src/main/resources/lens-errors.conf 236d678 
>   lens-cube/src/main/java/org/apache/lens/cube/error/LensCubeErrorCode.java 
> babe3de 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java
>  7e9133e 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Lavkesh Lahngir 

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

(Updated April 21, 2017, 11:40 a.m.)


Review request for lens.


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


Repository: lens


Description
---

1. Cancel estimates for other drivers when the estimate pool queue is full.
2. Return the appropriate error message


Diffs (updated)
-

  lens-api/src/main/resources/lens-errors.conf 236d678 
  lens-cube/src/main/java/org/apache/lens/cube/error/LensCubeErrorCode.java 
babe3de 
  
lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
 c6fbeda 
  
lens-server/src/test/java/org/apache/lens/server/common/FailingQueryDriver.java 
7e9133e 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
7d8c977 
  lens-server/src/test/resources/lens-site.xml 334a9c4 


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

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


Testing
---


Thanks,

Lavkesh Lahngir

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Puneet Gupta 

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




lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
Lines 1716 (patched)


Not sure if logging the driver name makes sense. Also Should we log this as 
error or warning is debatable ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
Lines 1722 (patched)


can we use for(RewriteEstimateRunnable r : runnables) instead ?



lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
Lines 1727 (patched)


Alsoe we can add a sugestion to "Please try again later".


- Puneet Gupta


On April 21, 2017, 7:25 a.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 21, 2017, 7:25 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   
> lens-server-api/src/test/java/org/apache/lens/server/api/driver/MockDriver.java
>  92724c6 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Re: Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Amareshwari Sriramadasu 

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




lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
Lines 1727 (patched)


Should have a proper error code associated and message should go to 
lens-errors.conf



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 2044-2046 (patched)


We can use FailingQueryDriver instead of changing server state like this.



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 2051-2054 (patched)


Can we call estimate api instead of this and validate the error response 
from api?



lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
Lines 2065 (patched)


Calling sleeping estimates in for loop will do than running them as 
Runnables.


- Amareshwari Sriramadasu


On April 21, 2017, 7:25 a.m., Lavkesh Lahngir wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58604/
> ---
> 
> (Updated April 21, 2017, 7:25 a.m.)
> 
> 
> Review request for lens.
> 
> 
> Bugs: LENS-1413
> https://issues.apache.org/jira/browse/LENS-1413
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> 1. Cancel estimates for other drivers when the estimate pool queue is full.
> 2. Return the appropriate error message
> 
> 
> Diffs
> -
> 
>   
> lens-server-api/src/test/java/org/apache/lens/server/api/driver/MockDriver.java
>  92724c6 
>   
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
>  c6fbeda 
>   
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
> 7d8c977 
>   lens-server/src/test/resources/lens-site.xml 334a9c4 
> 
> 
> Diff: https://reviews.apache.org/r/58604/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lavkesh Lahngir
> 
>

Review Request 58604: Handle Estimates failing with task rejection errors

2017-04-21 Thread Lavkesh Lahngir 

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

Review request for lens.


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


Repository: lens


Description
---

1. Cancel estimates for other drivers when the estimate pool queue is full.
2. Return the appropriate error message


Diffs
-

  
lens-server-api/src/test/java/org/apache/lens/server/api/driver/MockDriver.java 
92724c6 
  
lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
 c6fbeda 
  lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java 
7d8c977 
  lens-server/src/test/resources/lens-site.xml 334a9c4 


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


Testing
---


Thanks,

Lavkesh Lahngir