----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26999/#review58679 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/26999/#comment99767> Might be good to make it clear this is returning a gauge in the fn name, so maybe getRoleCountGauge or createRoleCountGauge? Should this also be a CachedGuage()? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/26999/#comment99770> Brief class comment - what is this class, is it thread safe, mention that it is a singleton sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/26999/#comment99763> I think you need to assign the new class here. ou should also make this function synchronized to ensure the class is thread safe. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/26999/#comment99768> Rather than passing in SentryStore, consider exposing addGuage()/registerGuage() and then moving this logic to where the SentryMetrics class is initialized. That removes a dependency from the SentryMetrics class. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/26999/#comment99764> Can init be called multiple times? Looks like it probably shouldn't be (comment on behavior). You might want to make this synchronized and add a flag isInitialized that skips initialization or throws an error if it has already happened. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/26999/#comment99765> maybe "registerMetricSet"? - Lenni Kuff 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 > >
