----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65097/#review195252 -----------------------------------------------------------
Can you add javadoc on all the new classes and public methods? We're trying to have better public API documentation. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Lines 168-169 (patched) <https://reviews.apache.org/r/65097/#comment274424> Didn't you mean HBASE_INDEXER and IndexerPrivilegeModel here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java Line 180 (original), 182 (patched) <https://reviews.apache.org/r/65097/#comment274426> Don't you need to add IndexerModelAuthorizables.from(keyValue) here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolIndexer.java Lines 167 (patched) <https://reviews.apache.org/r/65097/#comment274430> Incorrect 'solrctl' word key. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java Line 112 (original), 115 (patched) <https://reviews.apache.org/r/65097/#comment274427> Do you need to add the hbase indexer component here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java Line 124 (original), 127 (patched) <https://reviews.apache.org/r/65097/#comment274428> Do you need to add a hbase indexer service here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellIndexer.java Lines 128-146 (patched) <https://reviews.apache.org/r/65097/#comment274429> I see that the GenericPrivilegeConverter has information about other generic services (kafka, solr and sqoop), why to extend from GenericPrivilegeConverter instead of using it the same way kafka and solr does it? - Sergio Pena On Jan. 11, 2018, 2 p.m., Mano Kovacs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65097/ > ----------------------------------------------------------- > > (Updated Jan. 11, 2018, 2 p.m.) > > > Review request for sentry, Colm O hEigeartaigh and Sergio Pena. > > > Bugs: SENTRY-2023 > https://issues.apache.org/jira/browse/SENTRY-2023 > > > Repository: sentry > > > Description > ------- > > CLI tool to manage Hbase indexer permissions in Sentry Service. It combines > the common SentryShell commands with the configtool solr had for migrating > .ini files into Sentry Service. > > The patch depends on SENTRY-641. > > > Diffs > ----- > > sentry-provider/sentry-provider-db/pom.xml > 192f8c86d5b0065c2b26c94ddfc3f71678d90fa7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java > c13e000012b6b781d36e852bb4e059c880bdfeb9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java > c65b66d0103cce329c3f84ad51119bf096cdd503 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolIndexer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java > 1623f3869b13f7f34909b5c9e3d58ff3e9b0d942 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellIndexer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > 5fbc961ce88d50d97c355ab6988210413f64229c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolIndexer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellIndexer.java > PRE-CREATION > sentry-provider/sentry-provider-db/src/test/resources/indexer_case.ini > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/resources/indexer_config_import_tool.ini > PRE-CREATION > sentry-provider/sentry-provider-db/src/test/resources/indexer_invalid.ini > PRE-CREATION > > > Diff: https://reviews.apache.org/r/65097/diff/1/ > > > Testing > ------- > > > Thanks, > > Mano Kovacs > >
