> On Oct. 23, 2014, 7:44 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 274 > > <https://reviews.apache.org/r/26999/diff/1/?file=728226#file728226line274> > > > > "size" makes me think that it is going to return bytes. Consider > > renaming to: > > getGroupCount, getPrivCount, etc. > > That will also make it more clear that this is executing a query behind > > the scenes.
Done. > On Oct. 23, 2014, 7:44 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryHealthCheckServletContextListener.java, > > line 23 > > <https://reviews.apache.org/r/26999/diff/1/?file=728227#file728227line23> > > > > Add brief class comment on what this is/how this is used. Done. > On Oct. 23, 2014, 7:44 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 166 > > <https://reviews.apache.org/r/26999/diff/1/?file=728229#file728229line166> > > > > throw an error for an unhandled reporting type? This is handled just before calling initReporting. > On Oct. 23, 2014, 7:44 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, > > line 126 > > <https://reviews.apache.org/r/26999/diff/1/?file=728232#file728232line126> > > > > 8080 seems like a pretty common port and we might run into conflicts. > > You should probably choose a default that's higher and ensure it doesn't > > conflict with any other hadoop components (not sure if there is good > > documentation on this). Looked up ports on this doc: http://www.cloudera.com/content/cloudera/en/documentation/cloudera-manager/v5-latest/Cloudera-Manager-Installation-Guide/cm5ig_ports_cdh5.html And picked 51000, looks like easy to remember and close to HDFS ports. > On Oct. 23, 2014, 7:44 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 83 > > <https://reviews.apache.org/r/26999/diff/1/?file=728229#file728229line83> > > > > consider creating a new class (PolicyServiceMetrics?) that > > initializes/registers all the timers and exposes them as public instance > > members. > > > > That way we reduce the code in this class and make it easy to find > > where to add new metrics. Moved all the metrics specific stuff to class SentryMetrics. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26999/#review57966 ----------------------------------------------------------- On Oct. 27, 2014, 8:36 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26999/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2014, 8:36 p.m.) > > > Review request for sentry, Arun Suresh, Lenni Kuff, and Prasad Mujumdar. > > > Bugs: SENTRY-477 > https://issues.apache.org/jira/browse/SENTRY-477 > > > Repository: sentry > > > Description > ------- > > This patch is mainly adding a metrics infrastucture (using codahale) to > Sentry service to expose: > - Timer metrics for each API operation: # times called, max, min, mean and > histogram of time taken to serve the request. > - Aggregate metrics for size of meta data: #roles, #privileges, #groups in > the sentry db. > - JVM metrics: gc, memory, buffers, thread metrics > > These metrics are exposed in multiple ways: > - JMX > - Console > - We also add a basic Web UI using Jetty to serve this metric information. > Default on port 8080, and can be configured using property > "sentry.service.web.port" > Metrics reporting can be enabled by setting sentry.service.reporting to > jmx/console. > > We are also bumping up the jdk version to jdk7 as part of this patch as jdk6 > support is towards end of life. > > > Diffs > ----- > > pom.xml e172e92e023790adf66cc351593dba9ec2f08b36 > sentry-provider/sentry-provider-db/pom.xml > b4167e4576cf905171ce8e2b8dfa210fd64a2e90 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 350eb3267bd8a2922676815d3313b24b184d42be > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryHealthCheckServletContextListener.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetricsServletContextListener.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 67dc1f83aca8fba31a005f13d6a17a1de14d3729 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 40e8a0e02503ce504e7dbb4a732974be20db499c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 52eaeedcdd612d7ba0a36a93a93ac5dbe46f2cac > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > befecf4c41388648f28f0db441e9abe8ee971cd1 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > f251ebceca47d4a8aa9a7bf30e4f23d4a375ff82 > > Diff: https://reviews.apache.org/r/26999/diff/ > > > Testing > ------- > > Wrote unit tests for new functions introduced in SentryStore. Tested that the > web UI works as expected, attached the snapshots on the jira. > > > Thanks, > > Sravya Tirukkovalur > >
