> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-api/src/main/resources/lens-errors.conf, line 29
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382846#file1382846line29>
> >
> > Can we make this more readable ?
Keeping in sync with `javax.ws.rs.core.Response.Status`:
```
public static enum Status implements Response.StatusType {
OK(200, "OK"),
CREATED(201, "Created"),
ACCEPTED(202, "Accepted"),
NO_CONTENT(204, "No Content"),
MOVED_PERMANENTLY(301, "Moved Permanently"),
SEE_OTHER(303, "See Other"),
NOT_MODIFIED(304, "Not Modified"),
TEMPORARY_REDIRECT(307, "Temporary Redirect"),
BAD_REQUEST(400, "Bad Request"),
UNAUTHORIZED(401, "Unauthorized"),
FORBIDDEN(403, "Forbidden"),
NOT_FOUND(404, "Not Found"),
NOT_ACCEPTABLE(406, "Not Acceptable"),
CONFLICT(409, "Conflict"),
GONE(410, "Gone"),
PRECONDITION_FAILED(412, "Precondition Failed"),
UNSUPPORTED_MEDIA_TYPE(415, "Unsupported Media Type"),
INTERNAL_SERVER_ERROR(500, "Internal Server Error"),
SERVICE_UNAVAILABLE(503, "Service Unavailable");
```
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/BaseLensService.java, line
> > 307
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382853#file1382853line307>
> >
> > We need to consider the sitution where a user has submitted 100 queries
> > in 100 different sessions and closed session after each submission. If the
> > session limit for user is 100, and all the queries are either
> > queued/running, then he won't be able to open any more sessions, even
> > though he had issues closed on each opened session.
Moved this outside the if-else clause.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/BaseLensService.java, line
> > 343
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382853#file1382853line343>
> >
> > Should we use "SESSION_ID_NOT_PROVIDED" ?
Not throwing LensException here right now, since not all APIs return
LensAPIResult. Plus, this will add a `throws` declaration in a lot of methods
across classes cluttering the diff for this issue.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/metastore/MetastoreResource.java,
> > lines 66-68
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382855#file1382855line66>
> >
> > This may not be required. Check LENS-957 : Add GenericExceptionMapper
> > to map all non LensException as well.
Yes, didn't want to change signature of so many functions. Adding `throws`
declaration everywhere will clutter the diff. We can take that up as a cleanup.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java,
> > line 1314
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382856#file1382856line1314>
> >
> > Do we need to add the logic to delete the previously
> > formatted(partially/completely) result ,if any, in ResultFormatter?
Right now, queries get stuck in EXECUTED state. The number of such queries is
generally small. With this, the number will become 0, either the queries will
fail, or they'll succeed. Either way, that's a more desirable state to be in.
Not clubbing that logic in this jira right now, Can be taken up separately when
issues are found.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java,
> > line 2903
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382856#file1382856line2903>
> >
> > Not sure why this method is here. This method is only used in test
> > classes. Should we remove it and let test classes use SessionService to
> > close session ?
Removed based on your other comment.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryServiceResource.java,
> > line 73
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382857#file1382857line73>
> >
> > This is not required after lens-957
Again, didn't want to add `throws` in all the methods. Can be taken up
separately.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/QueryServiceResource.java,
> > line 77
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382857#file1382857line77>
> >
> > should we merge this with checkSessionId() ?
Can be done, but as part of a separate cleaup jira.
> On May 16, 2016, 9:31 a.m., Puneet Gupta wrote:
> > lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java,
> > lines 493-498
> > <https://reviews.apache.org/r/47174/diff/4/?file=1382858#file1382858line493>
> >
> > Should we move this code to BaseLensService#closeSession ?
> >
> > The If check will not be required then
Moved. As mentioned in your other comment.
- Rajat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47174/#review133295
-----------------------------------------------------------
On May 24, 2016, 7 p.m., Rajat Khandelwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47174/
> -----------------------------------------------------------
>
> (Updated May 24, 2016, 7 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-904
> https://issues.apache.org/jira/browse/LENS-904
>
>
> Repository: lens
>
>
> Description
> -------
>
> In the current scenario, if the queries are queued from lens side (because of
> throttling), then these queries fails on session close.
>
>
> Diffs
> -----
>
> lens-api/src/main/resources/lens-errors.conf
> 395d63b87b385607fbb0435bd99ab05b65ca51dd
> lens-client/src/test/java/org/apache/lens/client/TestLensClient.java
> c49b5e860c9c30c54e7a58c629e47ffe60709f62
> lens-driver-hive/src/main/java/org/apache/lens/driver/hive/HiveDriver.java
> 04d059d65f1ac0ee0f50691cde946d00cbdc57fe
>
> lens-server-api/src/main/java/org/apache/lens/server/api/SessionValidator.java
> PRE-CREATION
>
> lens-server-api/src/main/java/org/apache/lens/server/api/metastore/CubeMetastoreService.java
> 3e9f28661132366cd92837265a0a0f7119a24853
>
> lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java
> 15ed2229dc21a730140efa5a7297d9c0329cabbc
> lens-server/src/main/java/org/apache/lens/server/BaseLensService.java
> b96cdf0585b285e449dde0e77467f44cbda07d0a
>
> lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java
> 2443fecea303ed963bfcd82071f4ca69ded46227
>
> lens-server/src/main/java/org/apache/lens/server/metastore/MetastoreResource.java
> 4a6d779a50aa2f0180b0fd35e891dbb2ece770fd
>
> lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java
> 1b3a7c0f6dd9949d313d7c5920d0f0f1dcd18c0b
>
> lens-server/src/main/java/org/apache/lens/server/query/QueryServiceResource.java
> c70689b110462e9623e1e3b5d37af97270c673dc
>
> lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java
> 6c5e52d150bdbbb1075d9150901068bbd3400594
>
> lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java
> 82a4e15a6e5b12fb5f8ac8fe43076942266d3db5
> lens-server/src/test/java/org/apache/lens/server/LensJerseyTest.java
> b5d54829ce4c41145eda39702af9f26ed0958fde
> lens-server/src/test/java/org/apache/lens/server/TestServerRestart.java
> f3d72f4b371020602e5eea0e5c538d5fc1b40de9
>
> lens-server/src/test/java/org/apache/lens/server/metastore/TestMetastoreService.java
> 5424404567060009ca27edab814f9b6cf82a594a
>
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryConstraints.java
> 8493d8598adc07609be1ddf4de5734513db7b1eb
>
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryIndependenceFromSessionClose.java
> PRE-CREATION
>
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryService.java
> b6ec42268f9d71897e54beaaead3c2101b5dc06e
>
> lens-server/src/test/java/org/apache/lens/server/query/TestQueryServiceDuplicate.java
> 082840300e983b90bc2c43fb8555bf1860fe6bbc
>
> Diff: https://reviews.apache.org/r/47174/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rajat Khandelwal
>
>
