----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64949/#review194781 -----------------------------------------------------------
pom.xml Lines 513 (patched) <https://reviews.apache.org/r/64949/#comment273775> sentry-binding-hive-v2 is not used anymore by Sentry. It will be removed later in a future release. Is there something there that should be moved to sentry-binding-hive? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 48 (patched) <https://reviews.apache.org/r/64949/#comment273807> Could you add javadoc on this class? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 51 (patched) <https://reviews.apache.org/r/64949/#comment273779> Is this used somewhere? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 54 (patched) <https://reviews.apache.org/r/64949/#comment273780> Shouldn't we use AuthorizationComponent.HBASEINDEXER instead of this variable? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 57 (patched) <https://reviews.apache.org/r/64949/#comment273829> Can we use 'boolean' instead of 'Boolean'? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 79 (patched) <https://reviews.apache.org/r/64949/#comment273820> Can you mention this is an 'HBase indexer authorization provider' to help the developer know about it? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 130 (patched) <https://reviews.apache.org/r/64949/#comment273835> I see that kafka and solr uses 'authorize' as a method name. Should we stay consistent with them? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 134-137 (patched) <https://reviews.apache.org/r/64949/#comment273837> How should we handle the access denied exception here? Sentry has SentryAccessDeniedException class already. Is that useful? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 159 (patched) <https://reviews.apache.org/r/64949/#comment273816> Can you add more detailed to the error message about what the user should do to fix the problem? Here's an example: The HBase indexer resource '%s' does not exist. Use the 'hbaseindxer.hdfs.confdir' configuration variable to specify an existing resource directory. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 162 (patched) <https://reviews.apache.org/r/64949/#comment273817> Can you add more detailed to the error message about what the user should do to fix the problem? Here's an example: The HBase indexer resource '%s' is not a directory. Use the 'hbaseindxer.hdfs.confdir' configuration variable to specify an existing resource directory. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 165 (patched) <https://reviews.apache.org/r/64949/#comment273818> Can you add more detailed to the error message about what the user should do to fix the problem? Here's an example: The HBase indexer resource '%s' does not have read permissions. Change the directory permissions to allow the HBase indexer process to read or use the 'hbaseindxer.hdfs.confdir' configuration variable to specify an existing resource directory with read permissions. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 176-180 (patched) <https://reviews.apache.org/r/64949/#comment273822> Can you make this comment as a javadoc comment? Adding @param and @throw comments? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 182 (patched) <https://reviews.apache.org/r/64949/#comment273827> Could you add an error message that tells the user how to fix this issue if happens? I think we should get the kerberos info from the authzConf in this method so that you avoid throwing an exception if the kerberos credential are not specified on the configuration file. Btw, does this method need to be public? I don't see uded anywhere else. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 185 (patched) <https://reviews.apache.org/r/64949/#comment273828> Same comment from the keytabFile. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 187-189 (patched) <https://reviews.apache.org/r/64949/#comment273830> I think we can avoid the synchronized() block by using an AtomicBoolean for the kerberosInit instead? Seems once kerberosInit is true, then there's no way the rest of the syncrhonized block will be executed, right? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 194 (patched) <https://reviews.apache.org/r/64949/#comment273831> Can you mention that your attempting to acquire the kerberos ticket for the HBase indexer binding or something like that? Just to help for debugging purposes. sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java Lines 199 (patched) <https://reviews.apache.org/r/64949/#comment273833> Shouldn't we throw a checked exception instead of RuntimeException? Also, can we add an error message about what the user should do in case a login error happens? perhaps specify that they need to use X and Y configuration variables for the keryba and principal? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/conf/HBaseIndexerAuthzConf.java Lines 26 (patched) <https://reviews.apache.org/r/64949/#comment273839> Can you add javadoc in this class and methods? sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/conf/HBaseIndexerAuthzConf.java Lines 72 (patched) <https://reviews.apache.org/r/64949/#comment273782> Is this variable used somewhere? sentry-dist/src/license/THIRD-PARTY.properties Lines 1-21 (original), 1-18 (patched) <https://reviews.apache.org/r/64949/#comment273776> These THRID-PARTY.properties changes are generated everytime we compile our code. We will remove this dependency later, but can you remove these change from this patch? sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/IndexerWildcardPrivilege.java Line 124 (original), 124 (patched) <https://reviews.apache.org/r/64949/#comment273777> We have a CommonPrivilege.java class which handles these capital and lower cases scenarios. I think the CommonPrivilege was created to have a more generic API. Solr, Kafka and Hive use this privilege, should we replace the IndexerWildcardPrivilege for the CommonePrivilege instead? - Sergio Pena On Jan. 4, 2018, 1:51 p.m., Mano Kovacs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64949/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2018, 1:51 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-641 > https://issues.apache.org/jira/browse/SENTRY-641 > > > Repository: sentry > > > Description > ------- > > Binding for Lily Hbase Indexer, based on the implementation Gregory Chanan > created for CDH version of Sentry. The patch contains implementation that has > no dependency to Lily. > > > Diffs > ----- > > pom.xml c797181c8f123bc914ece27c13b70540978091bc > sentry-binding/pom.xml b12683c1c742c989ac118e968c71df50366a8557 > sentry-binding/sentry-binding-hbase-indexer/pom.xml PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/SentryHBaseIndexerAuthorizationException.java > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/conf/HBaseIndexerAuthzConf.java > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/test/java/org/apache/sentry/binding/hbaseindexer/TestHBaseIndexerAuthzBinding.java > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/test/resources/log4j.properties > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/test/resources/sentry-site-service.xml > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/test/resources/sentry-site.xml > PRE-CREATION > > sentry-binding/sentry-binding-hbase-indexer/src/test/resources/test-authz-provider.ini > PRE-CREATION > sentry-dist/pom.xml 63eb81c87172dc3237039b6d7641881b2831079f > sentry-dist/src/license/THIRD-PARTY.properties > 22429fc3d644ea961d59260b4a08eb380d7a4325 > > sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/IndexerWildcardPrivilege.java > 9e9a0d2b3afc22098980b5a54b6a9989ad035b6f > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java > 5dc2b5592bdfbb281b083bdc835d455d07df0d86 > > > Diff: https://reviews.apache.org/r/64949/diff/1/ > > > Testing > ------- > > > Thanks, > > Mano Kovacs > >
