> On Oct. 27, 2014, 5:49 p.m., Prasad Mujumdar wrote: > > Firstly, excellent work! It's great to see the reporting framework getting > > in place! > > A few comments/suggeestions at high level - > > - Should we handle the bluk loading client (Impala, proposed HDFS client > > etc) separately ? > > - Log a followup ticket for WebServer authentication > > > > A few minor comments below.
Thanks for the feedback Prasad! - Filed a jira for tracking authentication support for Sentry web server. - In terms of handling different client separately, right now we do not have the support unless there are some service APIs which are client specific. We may have to add support in APIs to identify the client? > On Oct. 27, 2014, 5:49 p.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-db/pom.xml, line 142 > > <https://reviews.apache.org/r/26999/diff/1/?file=728225#file728225line142> > > > > It would be good add it to the maven dependency management and define > > the versions in the root pom. Moved the versions to the root pom. > On Oct. 27, 2014, 5:49 p.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 285 > > <https://reviews.apache.org/r/26999/diff/1/?file=728226#file728226line285> > > > > I am not sure if it's a good idea to unconditionally commit the > > transaction. Should we have a rollback for error cases ? It is a read only transanction, so is there an advanatage in rolling back? > On Oct. 27, 2014, 5:49 p.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 127 > > <https://reviews.apache.org/r/26999/diff/1/?file=728229#file728229line127> > > > > Nit: Should this be moved to the server initialization instread of > > printing for every session ? Processor intialization happens only once at SentryService startup, so this will be printed only once, not once per session. Isnt it? > On Oct. 27, 2014, 5:49 p.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java, > > line 236 > > <https://reviews.apache.org/r/26999/diff/1/?file=728231#file728231line236> > > > > Nit: null check for serntryWebServer before stopping ? Done - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26999/#review58629 ----------------------------------------------------------- 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 > >
