> On Oct. 27, 2014, 9:14 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java, > > line 77 > > <https://reviews.apache.org/r/26999/diff/2/?file=734478#file734478line77> > > > > I think you need to assign the new class here. > > > > ou should also make this function synchronized to ensure the class is > > thread safe.
Ah, good catch! Thanks! > On Oct. 27, 2014, 9:14 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java, > > line 82 > > <https://reviews.apache.org/r/26999/diff/2/?file=734478#file734478line82> > > > > 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. Yeah, I took that approach earlier. But in that case we do not have all metrics defined in SentryMetrics class. As these gauges will be defined in SentryPolicyStoreProcessor. So it is a tradeoff. > On Oct. 27, 2014, 9:14 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 291 > > <https://reviews.apache.org/r/26999/diff/2/?file=734476#file734476line291> > > > > 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()? Fixed the naming. CachedGauge doesnt seem to be usable out of the box. Currently investigating, will do as a follow on jira. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26999/#review58679 ----------------------------------------------------------- On Oct. 27, 2014, 9:53 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, 9:53 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 > >
