----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26999/#review57966 -----------------------------------------------------------
Ship it! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/26999/#comment98879> "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. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryHealthCheckServletContextListener.java <https://reviews.apache.org/r/26999/#comment98886> Add brief class comment on what this is/how this is used. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26999/#comment98883> 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. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26999/#comment98884> num_roles -> role_count and others sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26999/#comment98882> throw an error for an unhandled reporting type? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26999/#comment98880> consider renaming variable to "timerContext" or maybe just "timer". "context" is pretty generic sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java <https://reviews.apache.org/r/26999/#comment98881> 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). sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/26999/#comment98887> Also test for case when there are no roles - Lenni Kuff On Oct. 21, 2014, 11:09 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26999/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 11:09 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/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/dbprovider/TestDatabaseProvider.java > 38fa69e83931a0ac43032f69872d2ca6fb4573eb > > 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 > >
