----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69554/#review211269 -----------------------------------------------------------
lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java Lines 617 (patched) <https://reviews.apache.org/r/69554/#comment296215> can this be moved to the if condition? this would ensure there is atleast one row lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java Lines 623 (patched) <https://reviews.apache.org/r/69554/#comment296216> Can this be shifted to final block? Also DBUtils.close(stmt) and DBUtils.close(rs) makes more sense. This applies here and everywhere else lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java Line 390 (original), 827 (patched) <https://reviews.apache.org/r/69554/#comment296217> while all of existing code in lensserverdao is using commons db utils, any particular reason we are using vanilla java code in the new code? to maintain consistency in the code, it seems better to use the db utils library to define the new apis as well, IMO lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java Lines 2258 (patched) <https://reviews.apache.org/r/69554/#comment296222> makes more sense to move this to all executequery apis than in createcontext api to be in sync with the api definition lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java Lines 2367 (patched) <https://reviews.apache.org/r/69554/#comment296223> How is this supposed to work? Given the scenario where Lens 2 is given a query Q1 from Lens 1, the db may still contain an older status which it could have updated before going down Who exactly is updating the latest status of Q1 in the db? lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java Lines 3680 (patched) <https://reviews.apache.org/r/69554/#comment296224> Why can't we put this code in existing validatesession in Baselensservice than defining a new api altogether? lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java Lines 431 (patched) <https://reviews.apache.org/r/69554/#comment296220> can't see any calls to this api anywhere Also from our offline discussion, from my understanding , both instances share a separate persistent state(on hdfs) and they read session state from db only when it can't find a given session id in its memory Why does it require changes in existing restoresession in the first place? lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java Lines 590 (patched) <https://reviews.apache.org/r/69554/#comment296219> Instead of making a db call for every session , we need to have some batching mechanism for better performance lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java Line 579 (original), 616 (patched) <https://reviews.apache.org/r/69554/#comment296221> While all the write operations on sessions are handled in this code, I am not able to see any changes in list and get kind of apis in this class For instance, what happens if getAllSessionParameters api gets called with a session id from other lens instance, in which case I assume , restoresessionfrom db needs to be called in the getSession api of BaseLensService, which doesn't seem to be handled here restoresessionfromdb might have to be called in other apis involving write operations as well, which I am unable to see lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java Line 615 (original), 616 (patched) <https://reviews.apache.org/r/69554/#comment296218> public interface Externalizable extends Serializable { - Rajitha R On Dec. 12, 2018, 8:21 a.m., Ankit Kailaswar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69554/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2018, 8:21 a.m.) > > > Review request for lens, Amareshwari Sriramadasu and Rajitha R. > > > Bugs: LENS-1538 > https://issues.apache.org/jira/browse/LENS-1538 > > > Repository: lens > > > Description > ------- > > Implementation > > Session > > We need to persist all client session in database. > This involve persisting client session’s id and configuration in mysql > server. persisting entire session object is not required. We need to do this > as first thing while creating user session. > For each request from client lens server should not validate session id > from its in memory map. It should rather validate it from persisted session > in mysql server. > > if > > session is present in mysql and not present in lens server’s in memory > map(session manager) then it should create a lens session with same session > id and add it to in memory map > > else If > > session is not present in mysql and present in server’s in memory map then it > signifies that user have initiated close session from other host and we will > need to remove this session from current host’s in memory map as well > > else if > > session is not present in mysql and in server’s in memory map then return > “invalid session”. > > Whenever session expired/closed then we need to remove this session from > lens server’s in memory map and mysql. > > > > Query > > For sync queries lens server will close connection as soon as it is > stopped or failed with appropriate failure message. Client will be retrying > the query. > For async-light queries and async-heavy queries lens server takes care of > rescheduling all queries which were in running or queued state at the time of > restart. we can use this to make query ids available with both servers. > For async queries we need to persist all non finished async user queries > in mysql server in new table current_query. > If user executes query on host1 and it goes down and nginx points to > host2 then user should be able to poll on query status. Lens server will > first check query id in its inmemory maps if it is not present then it will > check in “finished_query” table in mysql else in “current_query” table in > mysql. In this case we will continue showing old status of query since it is > scheduled by host1. As soon as host1 comes up it will reschedule these > queries and will change the status. Optionally we can have host2 to move > these queries in “allqueries” map of its query service which will take care > of recovering, in this case we don't need to wait for host1 to come up and > reschedule. > While moving query from ”finished_quey” map to “finished_query” table in > mysql we will need to remove it from “current_query” tables as well. > This flow will remain same for request ids/query ids generated for query > plan/estimate. > > > > Issue addressed > > Lens downtime will be zero in case of deployment and if primary fail. > User will not have to create a new session whenever switch happens > User will be able to get status of all async queries seamlessly. > > > Diffs > ----- > > > lens-server-api/src/main/java/org/apache/lens/server/api/events/AsyncEventListener.java > 94734658 > > lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryContext.java > a0d8e0ba > > lens-server-api/src/main/java/org/apache/lens/server/api/query/QueryExecutionService.java > a8031098 > > lens-server-api/src/main/java/org/apache/lens/server/api/session/SessionService.java > ccd2a3b7 > lens-server/src/main/java/org/apache/lens/server/BaseLensService.java > 9364872d > > lens-server/src/main/java/org/apache/lens/server/error/LensServerErrorCode.java > ef43371a > lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java > cc6ca7d4 > > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java > 07a2107a > > lens-server/src/main/java/org/apache/lens/server/query/QueryServiceResource.java > 47b40a8f > > lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java > f6d43d70 > > lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java > 7d81375c > > > Diff: https://reviews.apache.org/r/69554/diff/1/ > > > Testing > ------- > > Unit testing > > > Thanks, > > Ankit Kailaswar > >
