> On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java, > > line 831 > > <https://reviews.apache.org/r/31669/diff/1/?file=882715#file882715line831> > > > > Why is this required?
This is to pass unique id for query across the code flow. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java, > > line 836 > > <https://reviews.apache.org/r/31669/diff/1/?file=882715#file882715line836> > > > > Which stack is this? When each query looked at by driver, the metric needs to be different for each driver. This name capture the stack from which driver the code reached there. Will rename to driver stack name, if we dont anticipate any other stacks. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/LensMetricsRegistry.java, > > line 40 > > <https://reviews.apache.org/r/31669/diff/1/?file=882716#file882716line40> > > > > I don't think we should start metrics registry in server-api. It should > > be done in metrics service only. Server-API should not force any instances > > on dependent modules. Same goes for the static factory instance. If not > > possible to move at all, make these lazy allocations. Sure. To pass registry across different modules and methods, the easiest way to pass it around is through static instance. Otherwise we need to have class level holders and setter. And set them from LensService all over. I will see if static can be avoided by another means. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetrics.java, > > line 30 > > <https://reviews.apache.org/r/31669/diff/1/?file=882718#file882718line30> > > > > This and the above class have same Javadoc What is the difference? Will update doc > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetricsFactory.java, > > line 53 > > <https://reviews.apache.org/r/31669/diff/1/?file=882719#file882719line53> > > > > I wonder why we need more singletons. Do we really need more static > > variables? Is it not possible to contain them in some service? Will check > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetricsFactory.java, > > line 101 > > <https://reviews.apache.org/r/31669/diff/1/?file=882719#file882719line101> > > > > Null check repeated twice? Looks like. This was moved code from lens-server to lens-server-api. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetricsFactory.java, > > line 149 > > <https://reviews.apache.org/r/31669/diff/1/?file=882719#file882719line149> > > > > Are we appending request values to the metric name? Again, moved code from lens-server to lens-server-api. To answer your question, yes. For some methods which use multipurpose param, query() is used for all ESTIMATE/EXPLAIN/EXECUTE/EXECUTE_WITH_TIMEOUT > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/query/AbstractQueryContext.java, > > line 115 > > <https://reviews.apache.org/r/31669/diff/1/?file=882720#file882720line115> > > > > Can we use Query handle here instead of one more UUID? No. Query handle is present only for queries accepted for execution. Others like ESTIMATE and EXPLAIN, do not have a handle. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/model/LensContainerRequest.java, > > line 36 > > <https://reviews.apache.org/r/31669/diff/1/?file=882722#file882722line36> > > > > This comment is too abstract. I couldn't understand why this class is > > needed. Which server code is this useful for? What do you mean by model > > object? Is this model from the MVC pattern? Again, moved code from lens-server to lens-server-api. Not doing any changes. > On March 4, 2015, 7:07 a.m., Jaideep dhok wrote: > > lens-server-api/src/main/java/org/apache/lens/server/model/LensContainerRequest.java, > > line 71 > > <https://reviews.apache.org/r/31669/diff/1/?file=882722#file882722line71> > > > > Why IllegalArgumentException? Please include error message. Also, will > > this exception ovverride any HTTP errors? Again, moved code from lens-server to lens-server-api. Not doing any changes. - Amareshwari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31669/#review75153 ----------------------------------------------------------- On March 3, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31669/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 7:11 a.m.) > > > Review request for lens, Himanshu Gahlaut, Jaideep dhok, and Rajat Khandelwal. > > > Bugs: LENS-283 > https://issues.apache.org/jira/browse/LENS-283 > > > Repository: lens > > > Description > ------- > > Changes include : > > - Added query level configuration to instrument estimate flow > - Methods instrumented to create gauges. > - Moved metrics related common classes to lens-server-api sothat they can be > used in other modules > > > Diffs > ----- > > lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryRewriter.java > ba19125 > lens-cube/src/main/java/org/apache/lens/driver/cube/RewriteUtil.java > 0e9ca25 > lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java > 1257092 > lens-server-api/pom.xml 8472840 > > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java > ca87fa3 > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/LensMetricsRegistry.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetricGauge.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetrics.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/metrics/MethodMetricsFactory.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/query/AbstractQueryContext.java > f141428 > > lens-server-api/src/main/java/org/apache/lens/server/api/query/DriverSelectorQueryContext.java > 1c04952 > > lens-server-api/src/main/java/org/apache/lens/server/model/LensContainerRequest.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/model/LensResourceMethod.java > PRE-CREATION > lens-server/src/main/java/org/apache/lens/server/metrics/MethodMetrics.java > 6a60421 > > lens-server/src/main/java/org/apache/lens/server/metrics/MethodMetricsFactory.java > f035848 > > lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java > 3f20bfd > > lens-server/src/main/java/org/apache/lens/server/model/LensContainerRequest.java > 307ae10 > > lens-server/src/main/java/org/apache/lens/server/model/LensResourceMethod.java > a3dca46 > > lens-server/src/main/java/org/apache/lens/server/query/QueryExecutionServiceImpl.java > 15b9849 > lens-server/src/main/resources/lenssession-default.xml f145358 > > lens-server/src/test/java/org/apache/lens/server/metrics/TestResourceMethodMetrics.java > 9e1c23f > > lens-server/src/test/java/org/apache/lens/server/query/TestResultFormatting.java > a0abb0a > src/site/apt/admin/config.apt 6445fe7 > src/site/apt/admin/session-config.apt d1c36bb > > Diff: https://reviews.apache.org/r/31669/diff/ > > > Testing > ------- > > Pending unit tests. > > Will update full test run once done. > > > Thanks, > > Amareshwari Sriramadasu > >
