> 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 > >
