Re: Review Request 46058: SENTRY-480: Create import tool that will load policy file about Solr into the DB store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46058/#review128735 --- Fix it, then Ship it! LGTM otherwise. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java (lines 46 - 48) <https://reviews.apache.org/r/46058/#comment192201> Can't we put these files under test/resources? - Vamsee Yarlagadda On April 12, 2016, 12:38 a.m., Gregory Chanan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46058/ > --- > > (Updated April 12, 2016, 12:38 a.m.) > > > Review request for sentry, shen guoquan, Sravya Tirukkovalur, and Vamsee > Yarlagadda. > > > Repository: sentry > > > Description > --- > > Updates guoquan's original patch to the latest Sentry API's and layout. > > In particular the differences from the original patch are: > 1) Convert to the latest Generic APIs > 2) Is a tool like the SolrSentryShell supporting similar flags > 3) No longer builds a (SolrAuthz)Binding, only uses the file provider backend > and the validators. Thus, we don't require the binding which is tricky to get > right anyway (the binding really runs in the server...are we convinced we > have all the right system properties, etc to get it to work)? > 4) because of 3), doesn't pass the policy file via the configuration file, > takes it on the command line. We need the configuration file to specify the > service info; having both the policy file and the service info in there means > users would have to create a configuration file just for use of the tool. > 5) Adds some additional compatibility checking around the fact that role > names are lower cased in the service but at least the solr backend supported > cased role names. > > Like the SolrSentryShell, this should probably live in sole solr-specific > client module, but since that doesn't exist yet, I put it in the > sentry-provider-db, which is the same place the SolrSentryShell exists. > > > Diffs > - > > sentry-provider/sentry-provider-db/pom.xml > b6efd1f2b7f2178b46b8538858292d31ea60743e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolCommon.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java > e2dfdf13d0c5f665080fb1c35da8b1cb648019df > sentry-provider/sentry-provider-db/src/main/resources/solr_case.ini > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/resources/solr_config_import_tool.ini > PRE-CREATION > sentry-provider/sentry-provider-db/src/main/resources/solr_invalid.ini > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/46058/diff/ > > > Testing > --- > > Ran the unit tests successfully. > > > Thanks, > > Gregory Chanan > >
Re: Review Request 46548: SENTRY-1212: Small authorization and compatibility checking bugs in Sentry conversion tool
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46548/#review130181 --- Ship it! Ship It! - Vamsee Yarlagadda On April 22, 2016, 12:58 a.m., Gregory Chanan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46548/ > --- > > (Updated April 22, 2016, 12:58 a.m.) > > > Review request for sentry, Sravya Tirukkovalur and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > --- > > When testing the tool from SENTRY-480 on a kerberos cluster I ran into a > couple of small bugs: > 1) The client is created after the UGI is returned, so may be out of date > 2) The compat checker always throws an exception; this exception will list > zero errors, but it would cleaner if it only threw exceptions on errors. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > bf91f5260312644a15735a2174e88d5a0e5e61b6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java > 7149f17ae11b96fad279a652a2ea7d7848e97d59 > > Diff: https://reviews.apache.org/r/46548/diff/ > > > Testing > --- > > Ran the new test. > > > Thanks, > > Gregory Chanan > >
Re: Review Request 46912: SENTRY-1228: SimpleFileProviderBackend error message missing spaces
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46912/#review131389 --- Ship it! Ship It! - Vamsee Yarlagadda On May 2, 2016, 9:57 p.m., Gregory Chanan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46912/ > --- > > (Updated May 2, 2016, 9:57 p.m.) > > > Review request for sentry and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > --- > > Trivial change to clean up an error message. > > > Diffs > - > > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > 91fd4a3b37c145982bc395077df8778e941f99ed > > Diff: https://reviews.apache.org/r/46912/diff/ > > > Testing > --- > > > Thanks, > > Gregory Chanan > >
Re: Review Request 47084: SENTRY-1233: Logging improvements to SentryConfigToolSolr
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47084/#review132311 --- Ship it! Ship It! - Vamsee Yarlagadda On May 7, 2016, 1:26 a.m., Gregory Chanan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47084/ > --- > > (Updated May 7, 2016, 1:26 a.m.) > > > Review request for sentry and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > --- > > From working with this tool for a bit, I think the following additions would > be useful: > 1) The tool warns/errors on cased role names being converted to lower case, > but still prints the cased name in the log. For clarity these should also be > lower cased. > 2) When not importing, we should still print a message about what we would > do, i.e. a dry-run message. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > 22895eb938f8c9092d68472038ddbf93534594f5 > > Diff: https://reviews.apache.org/r/47084/diff/ > > > Testing > --- > > Manually tested, verified that the message is printed to the log. > > > Thanks, > > Gregory Chanan > >
Re: Review Request 53872: SENTRY-1525: Provide script to run Sentry directly from the repo
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53872/#review158051 --- Ship it! Ship It! - Vamsee Yarlagadda On Dec. 3, 2016, 5 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53872/ > --- > > (Updated Dec. 3, 2016, 5 a.m.) > > > Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan > kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1525: Provide script to run Sentry directly from the repo > > > Diffs > - > > bin/run_sentry.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/53872/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 53872: SENTRY-1525: Provide script to run Sentry directly from the repo
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53872/#review158052 --- Can we rename the file to say "run_local_sentry" to explictly state it is for developer use rather than getting confused about the use of it. - Vamsee Yarlagadda On Dec. 3, 2016, 5 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53872/ > --- > > (Updated Dec. 3, 2016, 5 a.m.) > > > Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan > kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1525: Provide script to run Sentry directly from the repo > > > Diffs > - > > bin/run_sentry.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/53872/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54338/#review158054 --- sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java (lines 188 - 194) <https://reviews.apache.org/r/54338/#comment228726> A general question outside of this jira. This still makes me question the fact that when an update notification gets invoked, there is a chance that the update has failed to process properly and all we are doing is to simply log the error and move on. How does the caller get the notification that something didn't go as expected so that they can take some action on it? - Vamsee Yarlagadda On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54338/ > --- > > (Updated Dec. 3, 2016, 6:52 a.m.) > > > Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1515: Cleanup exception handling in SentryStore > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 3695709e03e683afe6196def53883e37e4910a1c > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > 73872813fb37428529f674fef924f5a05d23c2f6 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java > 3d756c914a1d79db15ab66eaa657c96d70e0dd1c > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 2ee06f9f236694f87beb9466285bb6363a0007de > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > f717f38cb14ea7594f87ec6c6bf30b78241dfed6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54338/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- This change will ensure that multiple calls to the DB are replaced with a single call that looks for multiple matching values. More reference: Checkout "Collections" section under http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 Diff: https://reviews.apache.org/r/54464/diff/ Testing --- Verified the changes by running the unit tests of TestSentryStore and all 40 tests passed. ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-06 18:30:07.974 java[5930:877254] Unable to load realm info from SCDynamicStore Tests run: 40, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 16.984 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 40, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 38.016s [INFO] Finished at: Tue Dec 06 18:30:24 PST 2016 [INFO] Final Memory: 59M/660M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
> On Dec. 7, 2016, 9:46 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1258 > > <https://reviews.apache.org/r/54464/diff/1/?file=1578441#file1578441line1258> > > > > It is neat, but does it actually work? Yes, i tested this and the existing tests that has multiple groups and roles passed as well. Anyway will put up a dedicated test to verify this. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/#review158405 ------- On Dec. 7, 2016, 2:39 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54464/ > --- > > (Updated Dec. 7, 2016, 2:39 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > This change will ensure that multiple calls to the DB are replaced with a > single call that looks for multiple matching values. > > More reference: > Checkout "Collections" section under > http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54464/diff/ > > > Testing > --- > > Verified the changes by running the unit tests of TestSentryStore and all 40 > tests passed. > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-06 18:30:07.974 java[5930:877254] Unable to load realm info from > SCDynamicStore > Tests run: 40, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 16.984 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 40, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > ---- > [INFO] Total time: 38.016s > [INFO] Finished at: Tue Dec 06 18:30:24 PST 2016 > [INFO] Final Memory: 59M/660M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
> On Dec. 7, 2016, 9:15 p.m., kalyan kumar kalvagadda wrote: > > It's better to add some unit test each function that changes. Added a new test. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/#review158397 --- On Dec. 7, 2016, 2:39 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54464/ > --- > > (Updated Dec. 7, 2016, 2:39 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > This change will ensure that multiple calls to the DB are replaced with a > single call that looks for multiple matching values. > > More reference: > Checkout "Collections" section under > http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54464/diff/ > > > Testing > --- > > Verified the changes by running the unit tests of TestSentryStore and all 40 > tests passed. > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-06 18:30:07.974 java[5930:877254] Unable to load realm info from > SCDynamicStore > Tests run: 40, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 16.984 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 40, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > > [INFO] Total time: 38.016s > [INFO] Finished at: Tue Dec 06 18:30:24 PST 2016 > [INFO] Final Memory: 59M/660M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/ --- (Updated Dec. 8, 2016, 7:44 a.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Addressed Kalyan and Sasha's comments. Repository: sentry Description --- This change will ensure that multiple calls to the DB are replaced with a single call that looks for multiple matching values. More reference: Checkout "Collections" section under http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 64df6a5655cf2c121cb44f2274369fbe9d70ec83 Diff: https://reviews.apache.org/r/54464/diff/ Testing --- Verified the changes by running the unit tests of TestSentryStore and all 40 tests passed. ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-06 18:30:07.974 java[5930:877254] Unable to load realm info from SCDynamicStore Tests run: 40, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 16.984 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 40, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 38.016s [INFO] Finished at: Tue Dec 06 18:30:24 PST 2016 [INFO] Final Memory: 59M/660M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/ --- (Updated Dec. 8, 2016, 7:47 a.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- This change will ensure that multiple calls to the DB are replaced with a single call that looks for multiple matching values. More reference: Checkout "Collections" section under http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 64df6a5655cf2c121cb44f2274369fbe9d70ec83 Diff: https://reviews.apache.org/r/54464/diff/ Testing (updated) --- Verified the changes by running the unit tests of TestSentryStore and all 40 tests passed. ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-07 23:46:42.147 java[5054:534219] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.538 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 42.003s [INFO] Finished at: Wed Dec 07 23:47:00 PST 2016 [INFO] Final Memory: 59M/660M [INFO] ``` Thanks, Vamsee Yarlagadda
Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed. Change based on: https://reviews.apache.org/r/54525/ Should we remove the method as the whole and do the DB null check directly at the if clauses? Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 Diff: https://reviews.apache.org/r/54526/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1607 > > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607> > > > > So this is just the rename - right? The point is that getDbName is > > never NULL. Or did you intend to check for the actual __NULL__ string here? With the change suggested in https://reviews.apache.org/r/54525/, getDbName could also return a NULL value. So that's why i renamed the function to check if DB is null or not or we could completely replace the function call with a conditional statement at the if clauses. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review158572 ------- On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > --- > > (Updated Dec. 8, 2016, 9:02 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Renames the method isMultiActionsSupported to isDBNull to reflect the right > action being performed. > Change based on: https://reviews.apache.org/r/54525/ > > Should we remove the method as the whole and do the DB null check directly at > the if clauses? > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54526/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > ---- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/ --- (Updated Dec. 9, 2016, 1:16 a.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Addressed Sasha's comments. Repository: sentry Description --- This change will ensure that multiple calls to the DB are replaced with a single call that looks for multiple matching values. More reference: Checkout "Collections" section under http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 64df6a5655cf2c121cb44f2274369fbe9d70ec83 Diff: https://reviews.apache.org/r/54464/diff/ Testing --- Verified the changes by running the unit tests of TestSentryStore and all 40 tests passed. ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-07 23:46:42.147 java[5054:534219] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.538 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 42.003s [INFO] Finished at: Wed Dec 07 23:47:00 PST 2016 [INFO] Final Memory: 59M/660M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Dec. 12, 2016, 7:57 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Addressed Sasha's comments. Repository: sentry Description --- Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54409: SENTRY-1476: SentryStore is subject to JDQL injection
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54409/#review159226 --- sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (lines 2595 - 2602) <https://reviews.apache.org/r/54409/#comment230167> Can't we leverage the method used in SENTRY-1557 to make the query simple to construct? More reference for JDO filter constructs: http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ - Vamsee Yarlagadda On Dec. 6, 2016, 5:30 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54409/ > --- > > (Updated Dec. 6, 2016, 5:30 a.m.) > > > Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Sravya > Tirukkovalur, Vamsee Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1476: SentryStore is subject to JDQL injection > > > Diffs > - > > pom.xml f5134875420ed5a1156ae24092e5e203b10417c8 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 64df6a5655cf2c121cb44f2274369fbe9d70ec83 > > Diff: https://reviews.apache.org/r/54409/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54338/#review159242 --- Ship it! Ship It! - Vamsee Yarlagadda On Dec. 7, 2016, 11:42 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54338/ > --- > > (Updated Dec. 7, 2016, 11:42 p.m.) > > > Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1515: Cleanup exception handling in SentryStore > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 3695709e03e683afe6196def53883e37e4910a1c > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > 73872813fb37428529f674fef924f5a05d23c2f6 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java > 3d756c914a1d79db15ab66eaa657c96d70e0dd1c > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 2ee06f9f236694f87beb9466285bb6363a0007de > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > f717f38cb14ea7594f87ec6c6bf30b78241dfed6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54338/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54409: SENTRY-1476: SentryStore is subject to JDQL injection
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54409/#review159249 --- Ship it! LGTM otherwise - Vamsee Yarlagadda On Dec. 6, 2016, 5:30 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54409/ > --- > > (Updated Dec. 6, 2016, 5:30 a.m.) > > > Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Sravya > Tirukkovalur, Vamsee Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1476: SentryStore is subject to JDQL injection > > > Diffs > - > > pom.xml f5134875420ed5a1156ae24092e5e203b10417c8 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 64df6a5655cf2c121cb44f2274369fbe9d70ec83 > > Diff: https://reviews.apache.org/r/54409/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
> On Dec. 13, 2016, 6:28 p.m., Alexander Kolbasov wrote: > > 1) Do we have any other users of fromNullCol or it should be removed as > > well? > > 2) This relates to SENTRY-1540 since now isMultiActionsSupported() becomes > > a real code and this changes the existing semantics some. Is it Ok or not? bq. 1) Do we have any other users of fromNullCol or it should be removed as well? I verified that there are no users of fromNullCol and that's why it was removed in the last patch. bq. 2) This relates to SENTRY-1540 since now isMultiActionsSupported() becomes a real code and this changes the existing semantics some. Is it Ok or not? As part of this change we are only ensuring thrift objects has NULL attributes for the fields that are not set. And this still preserves the functionality of isMultiActionsSupported as they are only checking for NULL variables in thrift objects. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159035 --- On Dec. 12, 2016, 7:57 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > --- > > (Updated Dec. 12, 2016, 7:57 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > e8d0e2505c5b178cae3d27e7d85caa652f630a2d > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54808/#review159475 --- Ship it! Ship It! - Vamsee Yarlagadda On Dec. 16, 2016, 7:23 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54808/ > --- > > (Updated Dec. 16, 2016, 7:23 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1526 Sentry processed stays alive after being killed > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 578364933a3cdcf6c142b836360a83d322fe5c11 > > Diff: https://reviews.apache.org/r/54808/diff/ > > > Testing > --- > > Now process successfully dies after hitting ^C. > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Dec. 16, 2016, 7:51 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Limiting isMultiActionsSupported to always return true to preserve original behaviour and to avoid regressions being added by this patch. Will fix the method correctly in SENTRY-1540 Repository: sentry Description --- Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Review Request 54819: SENTRY-1526: Sentry processed stays alive after being killed (Fixing Mismerge) (Alexander Kolbasov via Vamsee Yarlagadda)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54819/ --- Review request for sentry and Alexander Kolbasov. Repository: sentry Description --- SENTRY-1526: Sentry processed stays alive after being killed (Fixing Mismerge) (Alexander Kolbasov via Vamsee Yarlagadda) Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 5e36e48eb5f47dc82b168eca914499ddef6b6516 Diff: https://reviews.apache.org/r/54819/diff/ Testing --- ```bash [INFO] Rat check: Summary of files. Unapproved: 0 unknown: 0 generated: 0 approved: 1 licence. [INFO] [INFO] Reactor Summary: [INFO] [INFO] Sentry SUCCESS [2.873s] [INFO] Sentry Core ... SUCCESS [0.206s] [INFO] Sentry Core Common SUCCESS [4.610s] [INFO] Sentry Core Model DB .. SUCCESS [1.375s] [INFO] Sentry Core Model Indexer . SUCCESS [0.777s] [INFO] Sentry Core Model Search .. SUCCESS [0.676s] [INFO] Sentry Core Model Sqoop ... SUCCESS [0.543s] [INFO] Sentry Core Model Kafka ... SUCCESS [0.553s] [INFO] Sentry Policies ... SUCCESS [0.098s] [INFO] Sentry Policy Common .. SUCCESS [0.773s] [INFO] Sentry Providers .. SUCCESS [0.135s] [INFO] Sentry Provider Common SUCCESS [0.709s] [INFO] Sentry Provider File .. SUCCESS [0.716s] [INFO] Sentry Policy Engine .. SUCCESS [0.308s] [INFO] Sentry Bindings ... SUCCESS [0.139s] [INFO] Sentry Hive Binding Configuration . SUCCESS [1.334s] [INFO] Hive follower for Sentry .. SUCCESS [1.517s] [INFO] Sentry HDFS ... SUCCESS [0.098s] [INFO] Sentry HDFS Common SUCCESS [4.235s] [INFO] Sentry Provider DB SUCCESS [11.961s] [INFO] Sentry Binding for Kafka .. SUCCESS [1.651s] [INFO] Sentry Provider Cache . SUCCESS [0.485s] [INFO] Sentry Hive Binding Common SUCCESS [1.948s] [INFO] Sentry Binding for Solr ... SUCCESS [1.206s] [INFO] Sentry Binding for Sqoop .. SUCCESS [1.194s] [INFO] Sentry Binding for Hive ... SUCCESS [1.446s] [INFO] Sentry Policy for Indexer . SUCCESS [0.549s] [INFO] Sentry Solr ... SUCCESS [0.066s] [INFO] Solr Sentry Core .. SUCCESS [1.430s] [INFO] Solr Sentry handler ... SUCCESS [1.613s] [INFO] Sentry Tests .. SUCCESS [0.194s] [INFO] Sentry Solr Tests . SUCCESS [3.024s] [INFO] Sentry Sqoop Tests SUCCESS [2.447s] [INFO] Sentry Kafka Tests SUCCESS [1.427s] [INFO] Sentry HDFS Service ... SUCCESS [1.463s] [INFO] Sentry HDFS Namenode Plugin ... SUCCESS [1.122s] [INFO] Sentry Hive Tests . SUCCESS [3.560s] [INFO] Sentry HDFS Dist .. SUCCESS [1.251s] [INFO] Sentry Distribution ... SUCCESS [16.520s] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 1:17.581s [INFO] Finished at: Fri Dec 16 12:35:23 PST 2016 [INFO] Final Memory: 183M/1509M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
> On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1449 > > <https://reviews.apache.org/r/54525/diff/3/?file=1587905#file1587905line1449> > > > > I'd suggest a simpler approach. > > > > For all these fields just do > > > > Privilege.setFoo(mSentryPrivilege.getFoo()) > > > > So all fields which are null in mSentryPrivilege will become null in > > TSentryPrivilege which is exactly what we want without any extra ifs. It looks like when we are trying to save a null string to the database (JDO) we seem to be passing the string "__NULL__" instead. So it is kinda of essential to do the check if the string is needed null by doing isNULL before setting to TSentryPrivilege. Here is where we are converting to MSentryPrivilege: https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1472 And toNULLCol will replace all null strings with "__NULL__" string https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1720 > On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1644 > > <https://reviews.apache.org/r/54525/diff/3/?file=1587905#file1587905line1644> > > > > I think this is in SENTRY-1540. Reworded the comment to mean that as part of SENTRY-1541, we wanted to ensure to preserve the original behaviour. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159486 --- On Dec. 16, 2016, 7:51 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > --- > > (Updated Dec. 16, 2016, 7:51 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 868e67720196f443cd281ec4e80ad552bf86e569 > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Dec. 16, 2016, 9:17 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Addressed Sasha's comments. Repository: sentry Description --- Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source
> On Dec. 19, 2016, 8:15 p.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1720 > > <https://reviews.apache.org/r/54525/diff/4/?file=1588250#file1588250line1720> > > > > It looks like fromNULLCol is handling null string representation > > "__NULL__" as well. If we remove processing of such string, we need to > > handle it when processing TSentryPriviege? Fail to do so, may bring API > > backward incompatibility. Good catch. I will see what i can do here. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159644 ------- On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > --- > > (Updated Dec. 16, 2016, 9:17 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 868e67720196f443cd281ec4e80ad552bf86e569 > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54525: SENTRY-1541: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Jan. 3, 2017, 7:13 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- It looks like making a change to the way thrift objects are created could lead into many unwanted regressions as this is being consumed in many end user clients. So this will revert to the original behavior but provides more information (Comments) stating the reasons why some of the string conversions are needed in SentryStore.java for more context. Summary (updated) - SENTRY-1541: Additional comments to clarify the intent of string manipulation methods in SentryStore.java Repository: sentry Description (updated) --- Provides more information on why some of the string conversions are required in SentryStore.java Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
> On Dec. 19, 2016, 8:15 p.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1720 > > <https://reviews.apache.org/r/54525/diff/4/?file=1588250#file1588250line1720> > > > > It looks like fromNULLCol is handling null string representation > > "__NULL__" as well. If we remove processing of such string, we need to > > handle it when processing TSentryPriviege? Fail to do so, may bring API > > backward incompatibility. > > Vamsee Yarlagadda wrote: > Good catch. I will see what i can do here. Reverted the change to the original behaviour. Just added more comments for easier understanding. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159644 ------- On Jan. 3, 2017, 7:13 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > --- > > (Updated Jan. 3, 2017, 7:13 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Provides more information on why some of the string conversions are required > in SentryStore.java > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 868e67720196f443cd281ec4e80ad552bf86e569 > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54525: SENTRY-1541: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
> On Dec. 16, 2016, 10:18 p.m., kalyan kumar kalvagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1644 > > <https://reviews.apache.org/r/54525/diff/4/?file=1588250#file1588250line1644> > > > > I'm just confused here. What are we acheiving here. All we are trying > > to do is retain the old behaviour which could be wrong. > > > > > > If we want to retain the old behaviour we rather not do any code > > changes. Reverted to the original behaviour as per the expressed regression concerns. Added more comments to some of the outlier methods for easier understanding in future. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159504 ------- On Jan. 3, 2017, 7:13 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > --- > > (Updated Jan. 3, 2017, 7:13 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Provides more information on why some of the string conversions are required > in SentryStore.java > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 868e67720196f443cd281ec4e80ad552bf86e569 > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Jan. 3, 2017, 7:46 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Summary (updated) - SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java Repository: sentry Description --- Provides more information on why some of the string conversions are required in SentryStore.java Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Jan. 3, 2017, 8:02 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Addressed Sasha's comments. Repository: sentry Description --- Provides more information on why some of the string conversions are required in SentryStore.java Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/ --- (Updated Jan. 3, 2017, 8:15 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Was already on it. Used HTML Formatting. Repository: sentry Description --- Provides more information on why some of the string conversions are required in SentryStore.java Diffs (updated) - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569 Diff: https://reviews.apache.org/r/54525/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 55094: SENTRY-1532: Sentry Web UI isn't working
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55094/#review160457 --- sentry-tests/sentry-tests-solr/pom.xml (line 58) <https://reviews.apache.org/r/55094/#comment231602> It looks the server is using latest version of Jetty. 8.1.19.v20160209 And in the solr/sentry tests we are pulling down a old one. Will this conflict with the other version being run? - Vamsee Yarlagadda On Jan. 4, 2017, 12:31 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55094/ > --- > > (Updated Jan. 4, 2017, 12:31 a.m.) > > > Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan > kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1532 > https://issues.apache.org/jira/browse/SENTRY-1532 > > > Repository: sentry > > > Description > --- > > SENTRY-1532: Sentry Web UI isn't working > > > Diffs > - > > sentry-service/sentry-service-server/pom.xml > 97bd326b0305303194a965a60b6afd0feefbbe0f > sentry-tests/sentry-tests-solr/pom.xml > a60b4eed4dde6c182b061681305013b3bd548cc3 > > Diff: https://reviews.apache.org/r/55094/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/ --- (Updated Jan. 5, 2017, 12:53 a.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Changes --- Function to check for both databases and tables in the correct way. Repository: sentry Description --- Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed. Change based on: https://reviews.apache.org/r/54525/ Should we remove the method as the whole and do the DB null check directly at the if clauses? Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb Diff: https://reviews.apache.org/r/54526/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1607 > > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607> > > > > So this is just the rename - right? The point is that getDbName is > > never NULL. Or did you intend to check for the actual __NULL__ string here? > > Vamsee Yarlagadda wrote: > With the change suggested in https://reviews.apache.org/r/54525/, > getDbName could also return a NULL value. So that's why i renamed the > function to check if DB is null or not or we could completely replace the > function call with a conditional statement at the if clauses. > > Alexander Kolbasov wrote: > It is with the fix for SENTRY-1541 but not with the current diff. > Actually, applying SENTRY-1541 changes the semantics here - suddenly this > function is no longer a dummy call. So we either want to preserve the > original semantics (and this means removing the function altogether) or > changing to new one (in which case we should be confident that it is correct). Reverted the function to it's original name and added support to check for tables as well. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review158572 ------- On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > --- > > (Updated Jan. 5, 2017, 12:53 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Renames the method isMultiActionsSupported to isDBNull to reflect the right > action being performed. > Change based on: https://reviews.apache.org/r/54525/ > > Should we remove the method as the whole and do the DB null check directly at > the if clauses? > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 3f3afb7080ee330fedd48f4d400553fe14d57deb > > Diff: https://reviews.apache.org/r/54526/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Review Request 55192: SENTRY-1586: [unit test] Race condition between metastore server/client could cause connection refused errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55192/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- SENTRY-1586: [unit test] Race condition between metastore server/client could cause connection refused errors Diffs - sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java bf43798133b280daeff5539e4e1a0b299bf59f16 Diff: https://reviews.apache.org/r/55192/diff/ Testing --- ```bash vamsee-MBP:sentry vamsee$ mvn clean -f sentry-tests/sentry-tests-hive/pom.xml test ... ... Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 42.164 sec - in org.apache.sentry.tests.e2e.metastore.TestMetastoreEndToEnd Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0 Running org.apache.sentry.tests.e2e.metastore.TestMetaStoreWithPigHCat Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 11.527 sec - in org.apache.sentry.tests.e2e.metastore.TestMetaStoreWithPigHCat Results : Tests run: 330, Failures: 0, Errors: 0, Skipped: 13 [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54798: SENTRY-1546 Generic Policy provides bad error messages for Sentry exceptions
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54798/#review160611 --- sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 273) <https://reviews.apache.org/r/54798/#comment231742> Please give a space delimeter after the trimmedRoleName otherwise the message could get a bit difficult to read. And pls address the same concern at other places as well. - Vamsee Yarlagadda On Dec. 16, 2016, 12:23 a.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54798/ > --- > > (Updated Dec. 16, 2016, 12:23 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Repository: sentry > > > Description > --- > > Added code changes to SentryStore to have proper message (where the > exceptions are originated). If this is done we can avoid decorating them > later. > I saw issue with exceptions created while role create/drop and made required > changes. > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 742798d > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 898632d > > Diff: https://reviews.apache.org/r/54798/diff/ > > > Testing > --- > > I have performed tests using Sentrytool Client. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/#review160612 --- sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1065) <https://reviews.apache.org/r/54454/#comment231744> nit. typo Feilds -> Fields sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1077) <https://reviews.apache.org/r/54454/#comment231745> nit. Same typo as stated above. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (lines 1080 - 1083) <https://reviews.apache.org/r/54454/#comment231748> Can't we leverage SentryStore.isNull() ? Or Strings.isNullOrEmpty()? - Vamsee Yarlagadda On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54454/ > --- > > (Updated Jan. 5, 2017, 4:50 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1548 Setting GrantOption to UNSET upsets Sentry > > I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should > be addressed together. Last patch was bit confusing as I had to remove the > changes done for SENTRY-1547. This diff has changes for both of them. > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 898632d > > Diff: https://reviews.apache.org/r/54454/diff/ > > > Testing > --- > > Verfied the changes using sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review160626 --- sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java (line 504) <https://reviews.apache.org/r/54947/#comment231766> Just wondering, it makes perfect sense to call pm.deletePersistent for the privileges that have empty roles but for the else clause, we seem to call pm.makePersistent. Is this really required? - The updated privilege is already saved right? If it is not, where are they saving the privilege data into the database before this change? - Vamsee Yarlagadda On Jan. 5, 2017, 4:20 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > --- > > (Updated Jan. 5, 2017, 4:20 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1556 Simplify privilege cleaning > > I made changes for the first approach by removing privileges moment they are > not associated to any role. > I have identified below scenarios which this will happen > When a role is deleted, we can see if the associated privileges are > associated to any other roles. All the privileges that are not associated to > any roles can be deleted from storage > When a privilege is revoked for a role, we can remove the privilege from > storage if it is not associated to any role > > Note: Once this approached is reviewed and accepted, we need not call > PrivCleaner periodically for cleanup > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 12245ec6d1b28863e4c1fd7b265e60204732f0e6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > 55b61ac07c6865402ffe4d0ff1690afb435deb95 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 4c3af7992c90ba6ce33ff38ca6c5a3eb3492dd8b > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 0484eaade5853d13bc6671c6e0648c14f76116c0 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 742798dca6b3ad8c9b9eee1eea5380001d1d0cd9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryPrivilege.java > PRE-CREATION > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 29134fec58e479e79aefb6cf96c6261698d64c08 > > Diff: https://reviews.apache.org/r/54947/diff/ > > > Testing > --- > > Added unit tests the scenarios mentioned in description. > Also tested the same with Sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 52795: SENTRY-1499: Add feature flag for using NotificationLog
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52795/#review160773 --- sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java (lines 121 - 128) <https://reviews.apache.org/r/52795/#comment231988> Why do we have to drop a sentry table privilege during create table? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java (lines 179 - 185) <https://reviews.apache.org/r/52795/#comment231989> Same question as above? Why does create commands involve in drop privileges? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java (line 417) <https://reviews.apache.org/r/52795/#comment231991> nit. How about Boolean.parseBoolean ? https://docs.oracle.com/javase/7/docs/api/java/lang/Boolean.html#parseBoolean(java.lang.String) sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java (line 109) <https://reviews.apache.org/r/52795/#comment231992> nit. Strings.isNullOrEmpty()? https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Strings.html#isNullOrEmpty-java.lang.String- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 163) <https://reviews.apache.org/r/52795/#comment231993> If notificationLogEnabled is set to false, then in the new logic we simply skip setting up the hmsFollowerExecutor which is not the case in the original code. I thought we should simply fall back to the original metastore listener than the notification log listener. - Vamsee Yarlagadda On Dec. 20, 2016, 12:21 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52795/ > --- > > (Updated Dec. 20, 2016, 12:21 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, kalyan kumar > kalvagadda, Li Li, and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1499: Add feature flag for using NotificationLog > > 1. Keep two SentryMetastorePostEventListeners. One for using notificationlog, > another for not(Used the original SentryMetastorePostEventListeners from > master > https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java). > 2. Add feature flag for using HMSFollower and > SentryMetastorePostEventListeners with notification log. > > Change-Id: I46a11d27ae0159a735b6049e9b7c78216d0e5346 > > > Diffs > - > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > 75190c1805162e11baf7c4553668693e4e6bb3c4 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > a9d49f86961f1ced1af40afcfe6172f884a23d44 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > d80fa1e71c165b7f1faf1c50c451e049d76b770b > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 61b24fa0e9f221b75ae343fc534b4d82fbce272a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java > c4be62db71e67099f22f3ba8991e03a32fb0f28a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java > 6f1886f942ca80ab4c356b81777d3ac336017292 > > Diff: https://reviews.apache.org/r/52795/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54729/#review160776 --- Fix it, then Ship it! LGTM otherwise. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java (lines 73 - 87) <https://reviews.apache.org/r/54729/#comment231994> Just curious to know how datanucleus leverages these overriden methods like hashCode, toString, equals ? - Vamsee Yarlagadda On Dec. 14, 2016, 2:35 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54729/ > --- > > (Updated Dec. 14, 2016, 2:35 a.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Vamsee Yarlagadda. > > > Repository: sentry > > > Description > --- > > Create schema for storing HMS path change and Sentry permission change. It > contains change ID, timestamp and the change event in JSON format. > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > 98349232bc658c39791e58b64949ecb975fff7a0 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java > 53243b44329997e64ab70db5e5d8c3614ab974d6 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java > 315d4b3a80b9ab3ac8704ecbab81876f141423f1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > fb5470f635e03c762beb5bc2c6b06641b2476ce9 > > Diff: https://reviews.apache.org/r/54729/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 55190: SENTRY-1583: Refactor ZK/Curator code
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55190/#review160806 --- Fix it, then Ship it! LGTM otherwise. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java (line 251) <https://reviews.apache.org/r/55190/#comment232033> Just curious to know how this works, if the server is elected as a leader this method gets called and blocked on the condition. When someone calls deactivate, the blocked thread would resume and simply set the status isLeader as false. But how are we telling the curator framework to remove the ZK znode that removes the leader status for the server? - Vamsee Yarlagadda On Jan. 6, 2017, 6:06 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55190/ > --- > > (Updated Jan. 6, 2017, 6:06 a.m.) > > > Review request for sentry, Colin Ma, Misha Dmitriev, Dapeng Sun, Hao Hao, > kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1583 > https://issues.apache.org/jira/browse/SENTRY-1583 > > > Repository: sentry > > > Description > --- > > - Restored HAContext class > - Removed Fencing support from HA > - Removed Activator, Activators, and LeaderStatus classes > - Renamed LeaderStatusAdaptor to LeaderStatusMonitor which is now the sole > class responsible for leader status monitoring. > > > Diffs > - > > pom.xml df26edf74c04fffcd9fbc3da07e949362eb94728 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > c094058142706a6b564c54fa69ddff5f1e2e5048 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java > f80605c4d68ef266a24e65d14f51066388b48417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java > c3df4d8a6568f236dc5e05ab190cd1f484f7967a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activators.java > 2926eeb97ec450f12d0d08086165c091709ead99 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java > 2467921b3cdc9ebd267d308d5cdcce52a836f207 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java > e87b3d13dcf2d10eb7ef00d21b8fa8846b4d16c0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > a9d49f86961f1ced1af40afcfe6172f884a23d44 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > d80fa1e71c165b7f1faf1c50c451e049d76b770b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestActivator.java > 60cfc735832433fb4dbeae1c2d617dd713fc3f3e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceMetrics.java > bc375e32685dcbcbf0b6398856dceced3de789e6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java > c796fabc21005479fa6afc687cf1df7af76538a9 > > Diff: https://reviews.apache.org/r/55190/diff/ > > > Testing > --- > > Performed manual testing with two instances of Sentry running. Tried the > following actions: > > - Sending SIGUSR2 to processes which changes leadership status (verified via > log messages and emtrics) > - Killing active server - new sserver is elected as a leader > - Restarting ZK service - a leader is elected once ZK service is back. > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
> On Jan. 9, 2017, 6:27 p.m., Mat Crocker wrote: > > should your || be an && ? > > this logic suggests its ok if the db is null, but the table name isn't The comment for this function says: "// Currently INSERT/SELECT/ALL are supported for Table and DB level privileges" So if the privilege has either DB or Table set, then they support multiple actions. So that's why it has to be || instead of &&. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review160920 ------- On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > --- > > (Updated Jan. 5, 2017, 12:53 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Renames the method isMultiActionsSupported to isDBNull to reflect the right > action being performed. > Change based on: https://reviews.apache.org/r/54525/ > > Should we remove the method as the whole and do the DB null check directly at > the if clauses? > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 3f3afb7080ee330fedd48f4d400553fe14d57deb > > Diff: https://reviews.apache.org/r/54526/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > ---- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/ --- (Updated Jan. 9, 2017, 8:15 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description (updated) --- 1. isMultiActionsSupported used to return true for all invocations as tPrivilege.getDbName() is never set to NULL. So we should change it instead to do a proper check for null - SentryStore.isNull() instead. 2. Currently this method is only dealing database name string but it should also check for table name strings as mentioned in the comments of the above method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level privileges". So fixes the method to handle tables as well. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb Diff: https://reviews.apache.org/r/54526/diff/ Testing --- ```bash vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false ... --- T E S T S --- Running org.apache.sentry.provider.db.service.persistent.TestSentryStore 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore Results : Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 44.421s [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 [INFO] Final Memory: 67M/687M [INFO] ``` Thanks, Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1624 > > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624> > > > > Do you know why isMultiActionsSupported is needed? The existing comment > > is not quite clear to me. Updated the description of the review to address your question. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review160558 ------- On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > --- > > (Updated Jan. 9, 2017, 8:15 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > 1. > isMultiActionsSupported used to return true for all invocations as > tPrivilege.getDbName() is never set to NULL. > So we should change it instead to do a proper check for null - > SentryStore.isNull() instead. > > 2. Currently this method is only dealing database name string but it should > also check for table name strings as mentioned in the comments of the above > method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level > privileges". So fixes the method to handle tables as well. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 3f3afb7080ee330fedd48f4d400553fe14d57deb > > Diff: https://reviews.apache.org/r/54526/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true
> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1624 > > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624> > > > > Do you know why isMultiActionsSupported is needed? The existing comment > > is not quite clear to me. > > Vamsee Yarlagadda wrote: > Updated the description of the review to address your question. This method is used to check which entities support multiple actions (QUERY, UPDATE, ALL). As per the comment, it looks like Database and Table are the ones that support all three. Do you think even Server, Column support all actions? If so, we can get rid of the entire method. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review160558 ------- On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > --- > > (Updated Jan. 9, 2017, 8:15 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > 1. > isMultiActionsSupported used to return true for all invocations as > tPrivilege.getDbName() is never set to NULL. > So we should change it instead to do a proper check for null - > SentryStore.isNull() instead. > > 2. Currently this method is only dealing database name string but it should > also check for table name strings as mentioned in the comments of the above > method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level > privileges". So fixes the method to handle tables as well. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 3f3afb7080ee330fedd48f4d400553fe14d57deb > > Diff: https://reviews.apache.org/r/54526/diff/ > > > Testing > --- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > --- > T E S T S > --- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > > [INFO] BUILD SUCCESS > [INFO] > -------- > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > > ``` > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 55092: SENTRY-1581: Provide Log4J metrics reporter
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55092/#review161122 --- Ship it! sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java (line 20) <https://reviews.apache.org/r/55092/#comment232361> nit. I thought we should avoid using the wildcard imports. - Vamsee Yarlagadda On Dec. 29, 2016, 11:20 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55092/ > --- > > (Updated Dec. 29, 2016, 11:20 p.m.) > > > Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1581 > https://issues.apache.org/jira/browse/SENTRY-1581 > > > Repository: sentry > > > Description > --- > > SENTRY-1581: Provide Log4J metrics reporter > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java > c477f2471d0300655f61877c29281cd1885c6d28 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 898632ddef6ac5eec7b0e240c596f57f427fda9d > > Diff: https://reviews.apache.org/r/55092/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
> On Jan. 5, 2017, 8:34 p.m., Vamsee Yarlagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java, > > line 504 > > <https://reviews.apache.org/r/54947/diff/1/?file=1590559#file1590559line504> > > > > Just wondering, it makes perfect sense to call pm.deletePersistent for > > the privileges that have empty roles but for the else clause, we seem to > > call pm.makePersistent. Is this really required? - The updated privilege is > > already saved right? If it is not, where are they saving the privilege data > > into the database before this change? > > kalyan kumar kalvagadda wrote: > Logic is writtern in makePersistent function of MSentryGMPrivilege which > is called when a privilege is added/deleted. > > I'm replacing "pm.makePersistent(MSentryGMPrivilege)" function call with > the above function so it has to handle both the cases. Thanks for clarifying. It makes sense after your explanation. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review160626 --- On Jan. 5, 2017, 4:20 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ------- > > (Updated Jan. 5, 2017, 4:20 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1556 Simplify privilege cleaning > > I made changes for the first approach by removing privileges moment they are > not associated to any role. > I have identified below scenarios which this will happen > When a role is deleted, we can see if the associated privileges are > associated to any other roles. All the privileges that are not associated to > any roles can be deleted from storage > When a privilege is revoked for a role, we can remove the privilege from > storage if it is not associated to any role > > Note: Once this approached is reviewed and accepted, we need not call > PrivCleaner periodically for cleanup > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 12245ec6d1b28863e4c1fd7b265e60204732f0e6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > 55b61ac07c6865402ffe4d0ff1690afb435deb95 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 4c3af7992c90ba6ce33ff38ca6c5a3eb3492dd8b > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 0484eaade5853d13bc6671c6e0648c14f76116c0 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 742798dca6b3ad8c9b9eee1eea5380001d1d0cd9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryPrivilege.java > PRE-CREATION > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 29134fec58e479e79aefb6cf96c6261698d64c08 > > Diff: https://reviews.apache.org/r/54947/diff/ > > > Testing > --- > > Added unit tests the scenarios mentioned in description. > Also tested the same with Sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 55094: SENTRY-1532: Sentry Web UI isn't working
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55094/#review161343 --- Ship it! Ship It! - Vamsee Yarlagadda On Jan. 11, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55094/ > --- > > (Updated Jan. 11, 2017, 1:35 a.m.) > > > Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan > kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1532 > https://issues.apache.org/jira/browse/SENTRY-1532 > > > Repository: sentry > > > Description > --- > > SENTRY-1532: Sentry Web UI isn't working > > > Diffs > - > > sentry-service/sentry-service-server/pom.xml > 97bd326b0305303194a965a60b6afd0feefbbe0f > sentry-tests/sentry-tests-solr/pom.xml > a60b4eed4dde6c182b061681305013b3bd548cc3 > > Diff: https://reviews.apache.org/r/55094/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: [ANNOUNCE] New Sentry committer - Alexander Kolbasov
Congrats Sasha. Looking forward to more contributions. Thanks, Vamsee On Mon, Jan 23, 2017 at 2:09 PM, Hao Hao wrote: > Hi Sentry community, > > I am excited to announce that Apache Sentry PMC has invited Alexander > Kolbasov > to be a committer in the project and he agreed! > > Alexander has made great contributions to this project. He has made > significant operational > enhancements to Sentry, including adding the protection for possible JDQL > injection, discover/fixing > Sentry store consistency issue, and improving stability of Sentry service. > He also shown an desire > to improve Sentry from many different perspectives, proposing the new > design for Sentry HA and > a interactive shell on the dev list and providing constructive feedbacks on > other folks reviews. > > Hoping to see continued community involvement from Alexander! > > Best, > Hao > -- Thanks, Vamsee
Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56191/
---
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar
kalvagadda, and Vadim Spector.
Repository: sentry
Description
---
SENTRY-1619: Fix the secure HMS connection code in HMSFollower
-- HMSFollower has bugs in implementing logic that's preventing secure HMS
connection from taking place.
1. Need to use the right principal and keytab to use for the connection.
2. Fix the while blocks to allow the connection setup to happen.
Diffs
-
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
749c2ce8f89fe5960af5a4b48ff45a38091350f4
Diff: https://reviews.apache.org/r/56191/diff/
Testing
---
Sentry server log:
```bash
2017-02-01 14:02:48,537 INFO org.apache.sentry.service.thrift.HMSFollower:
HMSFollower is being initialized
2017-02-01 14:02:50,545 INFO DataNucleus.Persistence: Property
datanucleus.cache.level2 unknown - will be ignored
2017-02-01 14:02:51,911 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:02:57,186 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:02:57,335 INFO org.apache.sentry.service.thrift.SentryService:
Attempting to start...
2017-02-01 14:02:57,345 INFO
org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new
Context
2017-02-01 14:02:57,403 INFO org.apache.sentry.service.thrift.SentryService:
ProcessorFactory being used:
org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessorFactory
2017-02-01 14:02:57,711 INFO DataNucleus.Persistence: Property
datanucleus.cache.level2 unknown - will be ignored
2017-02-01 14:02:58,969 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:02:59,035 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:02:59,533 INFO org.apache.sentry.hdfs.SentryPlugin: Sentry HDFS
plugin initialized !!
2017-02-01 14:02:59,561 INFO org.apache.sentry.service.thrift.SentryService:
ProcessorFactory being used:
org.apache.sentry.provider.db.generic.service.thrift.SentryGenericPolicyProcessorFactory
2017-02-01 14:02:59,699 INFO DataNucleus.Persistence: Property
datanucleus.cache.level2 unknown - will be ignored
2017-02-01 14:03:00,776 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:03:00,854 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections <
1. Setting to 20
2017-02-01 14:03:00,917 INFO org.apache.sentry.service.thrift.SentryService:
ProcessorFactory being used:
org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory
2017-02-01 14:03:00,917 INFO
org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory: Calling
registerProcessor from SentryHDFSServiceProcessorFactory
2017-02-01 14:03:00,945 INFO org.apache.sentry.service.thrift.SentryService:
Serving on nightly-1.gce.cloudera.com/172.31.112.33:8038
2017-02-01 14:03:01,133 INFO org.eclipse.jetty.server.Server:
jetty-7.6.16.v20140903
2017-02-01 14:03:01,159 INFO org.eclipse.jetty.server.handler.ContextHandler:
started o.e.j.s.h.ContextHandler{/,null}
2017-02-01 14:03:01,180 INFO org.eclipse.jetty.server.handler.ContextHandler:
started o.e.j.s.ServletContextHandler{/,null}
2017-02-01 14:03:01,211 INFO org.eclipse.jetty.server.AbstractConnector:
Started SelectChannelConnector@0.0.0.0:29000
2017-02-01 14:03:01,510 WARN org.apache.sentry.hdfs.SentryPlugin: Recieved
Authz Path FULL update [6]..
2017-02-01 14:04:23,514 INFO org.apache.sentry.service.thrift.HMSFollower:
Making a kerberos connection to HMS
2017-02-01 14:04:46,255 INFO org.apache.sentry.service.thrift.HMSFollower:
Using kerberos principal: sentry/nightly-1.gce.cloudera@gce.cloudera.com
2017-02-01 14:05:15,016 INFO
org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new
Context
2017-02-01 14:05:20,783 INFO org.apache.sentry.service.thrift.HMSFollower:
Established kerberos context, will now connect to HMS
2017-02-01 14:05:35,212 INFO hive.metastore: Trying to connect to metastore
with URI thrift://nightly-1.gce.cloudera.com:9083
2017-02-01 14:05:37,210 INFO hive.metastore: Opened a connection to metastore,
current connections: 1
2017-02-01 14:05:37,212 INFO hive.metastore: Connected to metastore.
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> On Feb. 1, 2017, 11:17 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 182 > > <https://reviews.apache.org/r/56191/diff/1/?file=1621825#file1621825line182> > > > > I trhink we should sleep here for a while before retrying. > > > > Also we shouldn't continue in this case - we should either retry > > forever or fail completely. It looks the HMS client itself has a retry logic defined in it. ```bash 2017-02-01 18:44:36,102 INFO org.apache.sentry.service.thrift.HMSFollower: Established kerberos context, will now connect to HMS 2017-02-01 18:44:49,646 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 18:44:51,604 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 18:44:51,605 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 18:44:52,605 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 18:44:52,623 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 18:44:52,623 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 18:44:53,624 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 18:44:53,644 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 18:44:53,644 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 18:44:54,644 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 18:44:54,661 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 18:44:54,661 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 18:44:55,661 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 18:44:55,674 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 18:44:55,674 INFO hive.metastore: Waiting 1 seconds before next connection attempt. ``` Although, we can still preserve the retry logic in our HMSFollower to back off considerably if connection setup fails using the HMS client to avoid too much network traffic. Modified code to give up after the re-tries so that the subsequent re-run of HMSFollower instantiates new connection as per the thread execution logic defined in SentryService.java (https://github.com/apache/sentry/blob/d5176b2ea2e80d51f49d9e13075d794436fbe504/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L161-L162) - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56191/#review163899 ------- On Feb. 1, 2017, 10:30 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56191/ > --- > > (Updated Feb. 1, 2017, 10:30 p.m.) > > > Review request for sentry, Alexander Kolbasov, Lei Xu, Hao Hao, kalyan kumar > kalvagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1619: Fix the secure HMS connection code in HMSFollower > -- HMSFollower has bugs in implementing logic that's preventing secure HMS > connection from taking place. > > 1. Need to use the right principal and keytab to use for the connection. > 2. Fix the while blocks to allow the connection setup to happen. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 749c2ce8f89fe5960af5a4b48ff45a38091350f4 > > Diff: https://reviews.apache.org/r/56191/diff/ > > > Testing > --- > > Sentry server log: > ```bash > 2017-02-01 14:02:48,537 INFO org.apache.sentry.service.thrift.HMSFollower: > HMSFollower is being initialized > 2017-02-01 14:02:50,545 INFO DataNucleus.Persistence: Property > datanucleus.cache.level2 unknown - will be ignored > 2017-02-01 14:02:51,911 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:57,186 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:57,335 INFO org.apache.sentry.service.thrift.SentryService: > Attempting to start... > 2017-02-01 14:02:57,345 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Logg
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
2017-02-01 14:05:37,212 INFO hive.metastore: Connected to metastore. ``` Thanks, Vamsee Yarlagadda
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
ft://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:45,449 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:45,450 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:46,450 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:46,451 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:46,451 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:50,455 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:50,458 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:50,458 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:51,458 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:51,460 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:51,460 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:52,460 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:52,463 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:52,463 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:53,463 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:53,465 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:53,465 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:54,465 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:54,468 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:54,468 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:14:07,218 ERROR org.apache.sentry.service.thrift.HMSFollower: HMSFollower cannot connect to HMS!! 2017-02-01 19:14:07,991 INFO org.apache.sentry.service.thrift.HMSFollower: Making a kerberos connection to HMS 2017-02-01 19:14:07,992 INFO org.apache.sentry.service.thrift.HMSFollower: Using kerberos principal: sentry/nightly-1.gce.cloudera@gce.cloudera.com 2017-02-01 19:14:07,993 INFO org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new Context 2017-02-01 19:14:07,998 INFO org.apache.sentry.service.thrift.HMSFollower: Established kerberos context, will now connect to HMS 2017-02-01 19:23:01,286 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:23:01,479 INFO hive.metastore: Opened a connection to metastore, current connections: 1 2017-02-01 19:23:01,480 INFO hive.metastore: Connected to metastore. 2017-02-01 19:23:01,480 INFO org.apache.sentry.service.thrift.HMSFollower: Secure connection established with HMS ``` Thanks, Vamsee Yarlagadda
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
to the MetaStore Server... 2017-02-01 19:13:43,444 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:44,444 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:44,447 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:44,447 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:45,447 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:45,449 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:45,450 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:46,450 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:46,451 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:46,451 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:50,455 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:50,458 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:50,458 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:51,458 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:51,460 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:51,460 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:52,460 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:52,463 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:52,463 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:53,463 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:53,465 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:53,465 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:54,465 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:54,468 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:54,468 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:14:07,218 ERROR org.apache.sentry.service.thrift.HMSFollower: HMSFollower cannot connect to HMS!! 2017-02-01 19:14:07,991 INFO org.apache.sentry.service.thrift.HMSFollower: Making a kerberos connection to HMS 2017-02-01 19:14:07,992 INFO org.apache.sentry.service.thrift.HMSFollower: Using kerberos principal: sentry/nightly-1.gce.cloudera@gce.cloudera.com 2017-02-01 19:14:07,993 INFO org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new Context 2017-02-01 19:14:07,998 INFO org.apache.sentry.service.thrift.HMSFollower: Established kerberos context, will now connect to HMS 2017-02-01 19:23:01,286 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:23:01,479 INFO hive.metastore: Opened a connection to metastore, current connections: 1 2017-02-01 19:23:01,480 INFO hive.metastore: Connected to metastore. 2017-02-01 19:23:01,480 INFO org.apache.sentry.service.thrift.HMSFollower: Secure connection established with HMS ``` Thanks, Vamsee Yarlagadda
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> On Feb. 2, 2017, 5:44 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> > line 73
> > <https://reviews.apache.org/r/56191/diff/3/?file=1622020#file1622020line73>
> >
> > Would be good to make retry times and sleep time configurable. Also
> > retries for 10 times, is it too much?
Now depending on the retry logic of Hive's HiveMetaStoreClient. So we no longer
need this.
- Vamsee
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56191/#review163935
-------
On Feb. 2, 2017, 6:34 p.m., Vamsee Yarlagadda wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56191/
> ---
>
> (Updated Feb. 2, 2017, 6:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Lei Xu, Hao Hao, kalyan kumar
> kalvagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> -- HMSFollower has bugs in implementing logic that's preventing secure HMS
> connection from taking place.
>
> 1. Need to use the right principal and keytab to use for the connection.
> 2. Fix the while blocks to allow the connection setup to happen.
>
>
> Diffs
> -
>
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> 749c2ce8f89fe5960af5a4b48ff45a38091350f4
>
> Diff: https://reviews.apache.org/r/56191/diff/
>
>
> Testing
> ---
>
> Sentry server log:
> ```bash
> 2017-02-01 14:02:48,537 INFO org.apache.sentry.service.thrift.HMSFollower:
> HMSFollower is being initialized
> 2017-02-01 14:02:50,545 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:02:51,911 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:57,186 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:57,335 INFO org.apache.sentry.service.thrift.SentryService:
> Attempting to start...
> 2017-02-01 14:02:57,345 INFO
> org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new
> Context
> 2017-02-01 14:02:57,403 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessorFactory
> 2017-02-01 14:02:57,711 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:02:58,969 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:59,035 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:59,533 INFO org.apache.sentry.hdfs.SentryPlugin: Sentry HDFS
> plugin initialized !!
> 2017-02-01 14:02:59,561 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.provider.db.generic.service.thrift.SentryGenericPolicyProcessorFactory
> 2017-02-01 14:02:59,699 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:03:00,776 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:03:00,854 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:03:00,917 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory
> 2017-02-01 14:03:00,917 INFO
> org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory: Calling
> registerProcessor from SentryHDFSServiceProcessorFactory
> 2017-02-01 14:03:00,945 INFO org.apache.sentry.service.thrift.SentryService:
> Serving on nightly-1.gce.cloudera.com/172.31.112.33:8038
> 2017-02-01 14:03:01,133 INFO org.eclipse.jetty.server.Server:
> jetty-7.6.16.v20140903
> 2017-02-01 14:03:01,159 INFO org.eclipse.jetty.server.handler.ContextHandler:
> started o.e.j.s.h.ContextHandler{/,null}
> 2017-02-01 14:03:01,180 INFO org.eclipse.jetty.server.handler.ContextHandler:
> started o.e.j.s.ServletContextHandler{/,null}
> 2017-02-01 14:03:01,211 INFO org.eclipse.jetty.server.AbstractConnector:
> Started SelectChannelConnector@0.0.0.0:29000
> 2017-02-01 14:03:01,510 WARN org.apache.sentry.hdfs.SentryPlugin
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> On Feb. 1, 2017, 11:17 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 182 > > <https://reviews.apache.org/r/56191/diff/1/?file=1621825#file1621825line182> > > > > I trhink we should sleep here for a while before retrying. > > > > Also we shouldn't continue in this case - we should either retry > > forever or fail completely. > > Vamsee Yarlagadda wrote: > It looks the HMS client itself has a retry logic defined in it. > ```bash > 2017-02-01 18:44:36,102 INFO > org.apache.sentry.service.thrift.HMSFollower: Established kerberos context, > will now connect to HMS > 2017-02-01 18:44:49,646 INFO hive.metastore: Trying to connect to > metastore with URI thrift://nightly-1.gce.cloudera.com:9083 > 2017-02-01 18:44:51,604 WARN hive.metastore: Failed to connect to the > MetaStore Server... > 2017-02-01 18:44:51,605 INFO hive.metastore: Waiting 1 seconds before > next connection attempt. > 2017-02-01 18:44:52,605 INFO hive.metastore: Trying to connect to > metastore with URI thrift://nightly-1.gce.cloudera.com:9083 > 2017-02-01 18:44:52,623 WARN hive.metastore: Failed to connect to the > MetaStore Server... > 2017-02-01 18:44:52,623 INFO hive.metastore: Waiting 1 seconds before > next connection attempt. > 2017-02-01 18:44:53,624 INFO hive.metastore: Trying to connect to > metastore with URI thrift://nightly-1.gce.cloudera.com:9083 > 2017-02-01 18:44:53,644 WARN hive.metastore: Failed to connect to the > MetaStore Server... > 2017-02-01 18:44:53,644 INFO hive.metastore: Waiting 1 seconds before > next connection attempt. > 2017-02-01 18:44:54,644 INFO hive.metastore: Trying to connect to > metastore with URI thrift://nightly-1.gce.cloudera.com:9083 > 2017-02-01 18:44:54,661 WARN hive.metastore: Failed to connect to the > MetaStore Server... > 2017-02-01 18:44:54,661 INFO hive.metastore: Waiting 1 seconds before > next connection attempt. > 2017-02-01 18:44:55,661 INFO hive.metastore: Trying to connect to > metastore with URI thrift://nightly-1.gce.cloudera.com:9083 > 2017-02-01 18:44:55,674 WARN hive.metastore: Failed to connect to the > MetaStore Server... > 2017-02-01 18:44:55,674 INFO hive.metastore: Waiting 1 seconds before > next connection attempt. > ``` > > Although, we can still preserve the retry logic in our HMSFollower to > back off considerably if connection setup fails using the HMS client to avoid > too much network traffic. > > Modified code to give up after the re-tries so that the subsequent re-run > of HMSFollower instantiates new connection as per the thread execution logic > defined in SentryService.java > (https://github.com/apache/sentry/blob/d5176b2ea2e80d51f49d9e13075d794436fbe504/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L161-L162) > > Alexander Kolbasov wrote: > You are right - since it is called via periodic callback, we have > automatic sleep/retry logic, so we can just remove it completely from > HMSFollower. Simplified the connection logic as well. We now rely on Hive's HiveMetaStoreClient for retry mechanism. Looking at the Hive code, we can tune retries and delay timer using the properties: 1. hive.metastore.connect.retries (Default is 3) 2. hive.metastore.client.connect.retry.delay(Default is 1 sec) We can change these to a more conservative numbers so that it better reflects our usecase. These properties come from hive-site.xml that is kept on the classpath of Sentry process so the user can make sure they reflect right settings. - Vamsee ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56191/#review163899 --- On Feb. 2, 2017, 6:34 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56191/ > --- > > (Updated Feb. 2, 2017, 6:34 p.m.) > > > Review request for sentry, Alexander Kolbasov, Lei Xu, Hao Hao, kalyan kumar > kalvagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1619: Fix the secure HMS connection code in HMSFollower > -- HMSFollower has bugs in implementing logic that's preventing secure HMS > connection from taking place. > > 1. Need to use the right princip
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
02-01 19:13:43,444 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:43,444 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:44,444 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:44,447 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:44,447 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:45,447 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:45,449 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:45,450 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:46,450 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:46,451 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:46,451 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:50,455 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:50,458 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:50,458 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:51,458 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:51,460 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:51,460 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:52,460 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:52,463 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:52,463 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:53,463 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:53,465 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:53,465 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:13:54,465 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:13:54,468 WARN hive.metastore: Failed to connect to the MetaStore Server... 2017-02-01 19:13:54,468 INFO hive.metastore: Waiting 1 seconds before next connection attempt. 2017-02-01 19:14:07,218 ERROR org.apache.sentry.service.thrift.HMSFollower: HMSFollower cannot connect to HMS!! 2017-02-01 19:14:07,991 INFO org.apache.sentry.service.thrift.HMSFollower: Making a kerberos connection to HMS 2017-02-01 19:14:07,992 INFO org.apache.sentry.service.thrift.HMSFollower: Using kerberos principal: sentry/nightly-1.gce.cloudera@gce.cloudera.com 2017-02-01 19:14:07,993 INFO org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new Context 2017-02-01 19:14:07,998 INFO org.apache.sentry.service.thrift.HMSFollower: Established kerberos context, will now connect to HMS 2017-02-01 19:23:01,286 INFO hive.metastore: Trying to connect to metastore with URI thrift://nightly-1.gce.cloudera.com:9083 2017-02-01 19:23:01,479 INFO hive.metastore: Opened a connection to metastore, current connections: 1 2017-02-01 19:23:01,480 INFO hive.metastore: Connected to metastore. 2017-02-01 19:23:01,480 INFO org.apache.sentry.service.thrift.HMSFollower: Secure connection established with HMS ``` Thanks, Vamsee Yarlagadda
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> On Feb. 2, 2017, 9:15 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> > line 171
> > <https://reviews.apache.org/r/56191/diff/4/?file=1622351#file1622351line171>
> >
> > Can you add a comment to saying SentryKerberosContext handles TGT
> > renewals?
Sure will add a comment to explain this.
- Vamsee
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56191/#review164016
-------
On Feb. 2, 2017, 6:34 p.m., Vamsee Yarlagadda wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56191/
> ---
>
> (Updated Feb. 2, 2017, 6:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Lei Xu, Hao Hao, kalyan kumar
> kalvagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> -- HMSFollower has bugs in implementing logic that's preventing secure HMS
> connection from taking place.
>
> 1. Need to use the right principal and keytab to use for the connection.
> 2. Fix the while blocks to allow the connection setup to happen.
>
>
> Diffs
> -
>
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> 749c2ce8f89fe5960af5a4b48ff45a38091350f4
>
> Diff: https://reviews.apache.org/r/56191/diff/
>
>
> Testing
> ---
>
> Sentry server log:
> ```bash
> 2017-02-01 14:02:48,537 INFO org.apache.sentry.service.thrift.HMSFollower:
> HMSFollower is being initialized
> 2017-02-01 14:02:50,545 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:02:51,911 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:57,186 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:57,335 INFO org.apache.sentry.service.thrift.SentryService:
> Attempting to start...
> 2017-02-01 14:02:57,345 INFO
> org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new
> Context
> 2017-02-01 14:02:57,403 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessorFactory
> 2017-02-01 14:02:57,711 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:02:58,969 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:59,035 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:02:59,533 INFO org.apache.sentry.hdfs.SentryPlugin: Sentry HDFS
> plugin initialized !!
> 2017-02-01 14:02:59,561 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.provider.db.generic.service.thrift.SentryGenericPolicyProcessorFactory
> 2017-02-01 14:02:59,699 INFO DataNucleus.Persistence: Property
> datanucleus.cache.level2 unknown - will be ignored
> 2017-02-01 14:03:00,776 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:03:00,854 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections
> < 1. Setting to 20
> 2017-02-01 14:03:00,917 INFO org.apache.sentry.service.thrift.SentryService:
> ProcessorFactory being used:
> org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory
> 2017-02-01 14:03:00,917 INFO
> org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory: Calling
> registerProcessor from SentryHDFSServiceProcessorFactory
> 2017-02-01 14:03:00,945 INFO org.apache.sentry.service.thrift.SentryService:
> Serving on nightly-1.gce.cloudera.com/172.31.112.33:8038
> 2017-02-01 14:03:01,133 INFO org.eclipse.jetty.server.Server:
> jetty-7.6.16.v20140903
> 2017-02-01 14:03:01,159 INFO org.eclipse.jetty.server.handler.ContextHandler:
> started o.e.j.s.h.ContextHandler{/,null}
> 2017-02-01 14:03:01,180 INFO org.eclipse.jetty.server.handler.ContextHandler:
> started o.e.j.s.ServletContextHandler{/,null}
> 2017-02-01 14:03:01,211 INFO org.eclipse.jetty.server.AbstractConnector:
> Started SelectChannelConnector@0.0.0.0:29000
> 2017-02-01 14:03:01,510 WARN org.apache.sentry.hdfs.SentryPlugin: Recieved
> Authz Path FULL update [6]..
>
> 2017-02-01 14:04:23,514 INFO org.apache.sentry.service.th
Re: Review Request 56191: SENTRY-1619: Fix the secure HMS connection code in HMSFollower
> On Feb. 2, 2017, 9:31 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 160 > > <https://reviews.apache.org/r/56191/diff/4/?file=1622351#file1622351line160> > > > > what is the reason for removing the retry logic? 1. Well re-trying SentryKerberosContext doesn't necessarily help if Kerberos setup fails for the first time, it is unlikely to pass with retries as well (based on approach used in SentryServer.java) 2. And the rery logic for setting up HMS connection is removed as Hive's HiveMetaStoreClient already handles it. > On Feb. 2, 2017, 9:31 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 136 > > <https://reviews.apache.org/r/56191/diff/4/?file=1622351#file1622351line136> > > > > I think we should provide the address of the HMS server instead of > > ServiceConstants.ServerConfig.RPC_ADDRESS_DEFAULT. Well this sep has nothing to do with HMS. We are picking up the principal to use for Sentry connection to HMS and for this we need to use sentry specific principal e.g: sentry/_h...@gce.cloudera.com And we try to expand the HOST to say somehting like nightly-1.gce.cloudera.com and for this we try to extract the RPC address using: conf.get(ServiceConstants.ServerConfig.RPC_ADDRESS, ServiceConstants.ServerConfig.RPC_ADDRESS_DEFAULT) So we attempt to get it from conf first before falling back to use the default RPC_ADDRESS_DEFAULT. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56191/#review164023 ------- On Feb. 2, 2017, 6:34 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56191/ > --- > > (Updated Feb. 2, 2017, 6:34 p.m.) > > > Review request for sentry, Alexander Kolbasov, Lei Xu, Hao Hao, kalyan kumar > kalvagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > SENTRY-1619: Fix the secure HMS connection code in HMSFollower > -- HMSFollower has bugs in implementing logic that's preventing secure HMS > connection from taking place. > > 1. Need to use the right principal and keytab to use for the connection. > 2. Fix the while blocks to allow the connection setup to happen. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 749c2ce8f89fe5960af5a4b48ff45a38091350f4 > > Diff: https://reviews.apache.org/r/56191/diff/ > > > Testing > --- > > Sentry server log: > ```bash > 2017-02-01 14:02:48,537 INFO org.apache.sentry.service.thrift.HMSFollower: > HMSFollower is being initialized > 2017-02-01 14:02:50,545 INFO DataNucleus.Persistence: Property > datanucleus.cache.level2 unknown - will be ignored > 2017-02-01 14:02:51,911 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:57,186 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:57,335 INFO org.apache.sentry.service.thrift.SentryService: > Attempting to start... > 2017-02-01 14:02:57,345 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new > Context > 2017-02-01 14:02:57,403 INFO org.apache.sentry.service.thrift.SentryService: > ProcessorFactory being used: > org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessorFactory > 2017-02-01 14:02:57,711 INFO DataNucleus.Persistence: Property > datanucleus.cache.level2 unknown - will be ignored > 2017-02-01 14:02:58,969 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:59,035 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:02:59,533 INFO org.apache.sentry.hdfs.SentryPlugin: Sentry HDFS > plugin initialized !! > 2017-02-01 14:02:59,561 INFO org.apache.sentry.service.thrift.SentryService: > ProcessorFactory being used: > org.apache.sentry.provider.db.generic.service.thrift.SentryGenericPolicyProcessorFactory > 2017-02-01 14:02:59,699 INFO DataNucleus.Persistence: Property > datanucleus.cache.level2 unknown - will be ignored > 2017-02-01 14:03:00,776 WARN com.jolbox.bonecp.BoneCPConfig: Max Connections > < 1. Setting to 20 > 2017-02-01 14:03:00,854
Review Request 56271: SENTRY-1621: HMSFollower to retry connecting to HMS upon connection loss
:01:31,451 INFO org.apache.sentry.service.thrift.HMSFollower: Hive full update initialization complete !! 2017-02-02 20:01:31,454 INFO org.apache.sentry.service.thrift.HMSFollower: After fetching hive full snapshot, Current NotificationID = CurrentNotificationEventId(eventId:0). 2017-02-02 20:01:31,454 INFO org.apache.sentry.service.thrift.HMSFollower: Successfully fetched hive full snapshot, Current NotificationID = CurrentNotificationEventId(eventId:0). 2017-02-02 20:01:31,516 INFO org.apache.sentry.service.thrift.HMSFollower: CurrentEventID = 0. Processing 0 events ``` Thanks, Vamsee Yarlagadda
Re: Review Request 56271: SENTRY-1621: HMSFollower to retry connecting to HMS upon connection loss
> On Feb. 3, 2017, 5:03 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 269 > > <https://reviews.apache.org/r/56271/diff/1/?file=1622914#file1622914line269> > > > > can client.close() throw any exception? If it does, you'll miss > > resetting of kerberos context. By the function definition, it doesn't have any checked exceptions. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56271/#review164087 ----------- On Feb. 3, 2017, 4:42 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56271/ > --- > > (Updated Feb. 3, 2017, 4:42 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > Looking at the existing logic of HMSFollower, upon the connection loss to HMS > the client never attempts to reconnect to HMS but rather just errors out with > "Broken pipe" indefinitely. > > ```bash > org.apache.thrift.transport.TTransportException: java.net.SocketException: > Broken pipe > at > org.apache.thrift.transport.TIOStreamTransport.flush(TIOStreamTransport.java:161) > ... > ... > Caused by: java.net.SocketException: Broken pipe > at java.net.SocketOutputStream.socketWrite0(Native Method) > at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:113) > ... > ... > ``` > > To improve this, we can extend HMSFollower to scan for SocketException errors > and if received, it can instead reconnect to HMS. > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > a1f970b1e6f14eedca057eb84df72de955d6f0c1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 1f05ba9cb6483251821f2965808dfd0711e70c63 > > Diff: https://reviews.apache.org/r/56271/diff/ > > > Testing > --- > > Ran the scenario of: > 1. Start with Sentry + HMS connected. > 2. Stop the HMS and verify the broken pipe errors reported on Sentry side and > it's attempts to establish a new connection. > 3. Start the HMS and verify the subsequent attempt of HMS client connection > succeeds and processes transactions. > > ```bash > ## FIRST STEP > ## > 2017-02-02 19:59:43,724 INFO org.apache.sentry.service.thrift.HMSFollower: > Making a kerberos connection to HMS > 2017-02-02 19:59:43,729 INFO org.apache.sentry.service.thrift.HMSFollower: > Using kerberos principal: > sentry/vamsee-sentry-1.vpc.cloudera@vpc.cloudera.com > 2017-02-02 19:59:43,729 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new > Context > 2017-02-02 19:59:43,753 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Sentry Ticket renewer > thread started > 2017-02-02 19:59:43,760 INFO hive.metastore: Trying to connect to metastore > with URI thrift://vamsee-sentry-1.vpc.cloudera.com:9083 > 2017-02-02 19:59:43,864 INFO hive.metastore: Opened a connection to > metastore, current connections: 1 > 2017-02-02 19:59:43,865 INFO hive.metastore: Connected to metastore. > 2017-02-02 19:59:43,865 INFO org.apache.sentry.service.thrift.HMSFollower: > Secure connection established with HMS > 2017-02-02 19:59:43,865 INFO org.apache.sentry.service.thrift.HMSFollower: > HMSFollower of Sentry successfully connected to HMS > 2017-02-02 19:59:43,978 INFO org.apache.sentry.service.thrift.HMSFollower: > Before fetching hive full snapshot, Current NotificationID = > CurrentNotificationEventId(eventId:0). > 2017-02-02 19:59:45,005 INFO org.apache.sentry.service.thrift.HMSFollower: > Hive full update initialization complete !! > 2017-02-02 19:59:45,008 INFO org.apache.sentry.service.thrift.HMSFollower: > After fetching hive full snapshot, Current NotificationID = > CurrentNotificationEventId(eventId:0). > 2017-02-02 19:59:45,009 INFO org.apache.sentry.service.thrift.HMSFollower: > Successfully fetched hive full snapshot, Current NotificationID = > CurrentNotificationEventId(eventId:0). > 2017-02-02 19:59:45,096 INFO org.apache.sentry.service.thrift.HM
Re: Review Request 56271: SENTRY-1621: HMSFollower to retry connecting to HMS upon connection loss
> On Feb. 3, 2017, 8:28 p.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 265 > > <https://reviews.apache.org/r/56271/diff/1/?file=1622914#file1622914line265> > > > > Could there be any other exception impiles needs to reconnect to HMS? SocketException should handle all possible cases that has to do with unable to access socket. https://docs.oracle.com/javase/7/docs/api/java/net/SocketException.html Atleast this should cover a good set of cases to begin with. We can expand more as we see it but more or less all connection related issues must be caught by this. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56271/#review164158 ------- On Feb. 3, 2017, 4:42 a.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56271/ > --- > > (Updated Feb. 3, 2017, 4:42 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > Looking at the existing logic of HMSFollower, upon the connection loss to HMS > the client never attempts to reconnect to HMS but rather just errors out with > "Broken pipe" indefinitely. > > ```bash > org.apache.thrift.transport.TTransportException: java.net.SocketException: > Broken pipe > at > org.apache.thrift.transport.TIOStreamTransport.flush(TIOStreamTransport.java:161) > ... > ... > Caused by: java.net.SocketException: Broken pipe > at java.net.SocketOutputStream.socketWrite0(Native Method) > at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:113) > ... > ... > ``` > > To improve this, we can extend HMSFollower to scan for SocketException errors > and if received, it can instead reconnect to HMS. > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > a1f970b1e6f14eedca057eb84df72de955d6f0c1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 1f05ba9cb6483251821f2965808dfd0711e70c63 > > Diff: https://reviews.apache.org/r/56271/diff/ > > > Testing > --- > > Ran the scenario of: > 1. Start with Sentry + HMS connected. > 2. Stop the HMS and verify the broken pipe errors reported on Sentry side and > it's attempts to establish a new connection. > 3. Start the HMS and verify the subsequent attempt of HMS client connection > succeeds and processes transactions. > > ```bash > ## FIRST STEP > ## > 2017-02-02 19:59:43,724 INFO org.apache.sentry.service.thrift.HMSFollower: > Making a kerberos connection to HMS > 2017-02-02 19:59:43,729 INFO org.apache.sentry.service.thrift.HMSFollower: > Using kerberos principal: > sentry/vamsee-sentry-1.vpc.cloudera@vpc.cloudera.com > 2017-02-02 19:59:43,729 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Logging in with new > Context > 2017-02-02 19:59:43,753 INFO > org.apache.sentry.service.thrift.SentryKerberosContext: Sentry Ticket renewer > thread started > 2017-02-02 19:59:43,760 INFO hive.metastore: Trying to connect to metastore > with URI thrift://vamsee-sentry-1.vpc.cloudera.com:9083 > 2017-02-02 19:59:43,864 INFO hive.metastore: Opened a connection to > metastore, current connections: 1 > 2017-02-02 19:59:43,865 INFO hive.metastore: Connected to metastore. > 2017-02-02 19:59:43,865 INFO org.apache.sentry.service.thrift.HMSFollower: > Secure connection established with HMS > 2017-02-02 19:59:43,865 INFO org.apache.sentry.service.thrift.HMSFollower: > HMSFollower of Sentry successfully connected to HMS > 2017-02-02 19:59:43,978 INFO org.apache.sentry.service.thrift.HMSFollower: > Before fetching hive full snapshot, Current NotificationID = > CurrentNotificationEventId(eventId:0). > 2017-02-02 19:59:45,005 INFO org.apache.sentry.service.thrift.HMSFollower: > Hive full update initialization complete !! > 2017-02-02 19:59:45,008 INFO org.apache.sentry.service.thrift.HMSFollower: > After fetching hive full snapshot, Current NotificationID = > CurrentNotificationEventId(eventId:0). > 2017-02-02 19:59:45,009 INFO
Re: Review Request 56134: SENTRY-1593 and SENTRY-1592 Implementing client failover for Generic policy clients and namenode clients
portConstants.java (lines 43 - 45) <https://reviews.apache.org/r/56134/#comment235759> Which parameter are we referring to? SECURITY_MODE? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (lines 46 - 70) <https://reviews.apache.org/r/56134/#comment235758> Why can't these constants be exposed as "public"? It looks like that's how even Hadoop implements it so that the references would be a lot easier. http://github.mtv.cloudera.com/CDH/hadoop/blob/cdh5-2.6.0/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/PolicyServiceTransportConstants.java (lines 59 - 70) <https://reviews.apache.org/r/56134/#comment235764> Same comment as my other comments on exposing these using helper methods. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (lines 59 - 60) <https://reviews.apache.org/r/56134/#comment235835> We should rather be using ServiceConstants.ServerConfig kerberos constants to keep in sync rather than duplicating constants. This applies to all the other places where we refer to kerberos constants. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (lines 62 - 84) <https://reviews.apache.org/r/56134/#comment235763> Yeah I agree about the comment made by Sasha. I don't think we should be exposing constants using helper methods. Example hadoop configs: http://github.mtv.cloudera.com/CDH/hadoop/blob/cdh5-2.6.0/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java (line 34) <https://reviews.apache.org/r/56134/#comment235765> Remove this? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java (lines 40 - 45) <https://reviews.apache.org/r/56134/#comment235772> Reason for commenting this out. sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java (line 58) <https://reviews.apache.org/r/56134/#comment235766> We shouldn't instantiate a constant class but rather directly refer to the static final variables of the class. sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java (lines 59 - 62) <https://reviews.apache.org/r/56134/#comment235847> How come these two are just the same? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java (line 196) <https://reviews.apache.org/r/56134/#comment235768> Remove? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (lines 486 - 493) <https://reviews.apache.org/r/56134/#comment235769> If it is no longer needed, can we remove it? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (lines 18 - 33) <https://reviews.apache.org/r/56134/#comment235770> Please cleanup the sections that are no longer needed. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (lines 58 - 62) <https://reviews.apache.org/r/56134/#comment235771> Reason for commenting this out. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1957) <https://reviews.apache.org/r/56134/#comment235850> typo? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 27) <https://reviews.apache.org/r/56134/#comment235851> Remove this. sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql (line 15) <https://reviews.apache.org/r/56134/#comment235852> Can you explain the motivation behind changing this value? - Vamsee Yarlagadda On Feb. 1, 2017, 9:11 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56134/ > --- > > (Updated Feb. 1, 2017, 9:11 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description >
Re: Review Request 55744: SENTRY-1559 Remove fencing support
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55744/#review164408 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 5, 2017, 5:24 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55744/ > --- > > (Updated Feb. 5, 2017, 5:24 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1559 > https://issues.apache.org/jira/browse/SENTRY-1559 > > > Repository: sentry > > > Description > --- > > SENTRY-1559 Remove fencing support > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java > 77aaac5fd14ac7d3b721261b898120bf15cc0daa > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java > 93fa92ce79aa0b57f3937d21013d73b051eab5dd > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java > 7c080fab53d902eb6ce909139cf6f0a9c288ba54 > > Diff: https://reviews.apache.org/r/55744/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 55999: SENTRY-1602 Code cleanup for Sentry JSON message factory for hive notifications
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55999/#review164411 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 4, 2017, 5:42 a.m., Nachiket Vaidya wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55999/ > --- > > (Updated Feb. 4, 2017, 5:42 a.m.) > > > Review request for sentry, Alexander Kolbasov and Hao Hao. > > > Bugs: SENTRY-1602 > https://issues.apache.org/jira/browse/SENTRY-1602 > > > Repository: sentry > > > Description > --- > > SENTRY-1602 Code cleanup for Sentry JSON message factory for hive > notifications > > > Diffs > - > > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java > 890186ba7eecd4aace280d9a1b56a8f01b625b77 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java > 76211c35f4f5ed158b5c3052f7288cd0083f0d52 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java > c0c469c1b1bfcd09494bb611a4a07911f5ef3192 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java > 99eb67a61363616af663a9be579b2e3a3344fd69 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java > 6e59e2568c8fb3526cd7a8ce29b08f5d8f5b0d62 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java > ba19cbe42a242ea74f3ea9bf424e799e95d17b3f > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java > 57d11d22f2f7c099c8dd354a207dbb297e39be3f > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java > 05f83f7effee07204e7b0597ac6faf1e0e2e6cdf > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java > 2ab61f7ad5d30a1edb454c3cb239532c736b52e6 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java > 7005776425a65ab5fdb4b959391c3558000406f4 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java > b645c4504ce6e768b31e6f7a1a39b60f73e1ac64 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java > 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java > 6f1886f942ca80ab4c356b81777d3ac336017292 > > Diff: https://reviews.apache.org/r/55999/diff/ > > > Testing > --- > > Ran unit test cases and they passed. > > > Thanks, > > Nachiket Vaidya > >
Re: Review Request 56000: SENTRY-1604 Sentry JSON message factory: Need more information in alter partition event
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56000/#review164413 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 4, 2017, 5:48 a.m., Nachiket Vaidya wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56000/ > --- > > (Updated Feb. 4, 2017, 5:48 a.m.) > > > Review request for sentry, Alexander Kolbasov and Hao Hao. > > > Bugs: SENTRY-1604 > https://issues.apache.org/jira/browse/SENTRY-1604 > > > Repository: sentry > > > Description > --- > > SENTRY-1604 Sentry JSON message factory: Need more information in alter > partition event > > > Diffs > - > > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java > 99eb67a61363616af663a9be579b2e3a3344fd69 > > sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java > 00e7db8ea4ccf92dae58869fcb21e6e2fcb27103 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java > 567b4c86339a9494647dccc980df2b427225907f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestDBNotificationListenerInBuiltDeserializer.java > 56e19c4d371918975fda127bf6f2fd32d98ed328 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java > 6f1886f942ca80ab4c356b81777d3ac336017292 > > Diff: https://reviews.apache.org/r/56000/diff/ > > > Testing > --- > > Added unit test cases. > > > Thanks, > > Nachiket Vaidya > >
Re: Review Request 55904: SENTRY-1612: HMSFollower should persist full HMS snapshot into SentryDB if there is not one.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55904/#review164419 --- Ship it! Ship It! - Vamsee Yarlagadda On Jan. 27, 2017, 1:03 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55904/ > --- > > (Updated Jan. 27, 2017, 1:03 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > --- > > After reads a point-in-time full HMSPaths snapshot from HMS and record the > corresponding > notification ID, HMSFollwer should persist it into Sentry DB. > > Otherwise if a full snapshot is stored in DB, read the corresponding > currentEventID from sentry store. > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > a1f970b1e6f14eedca057eb84df72de955d6f0c1 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java > 0bb6f665ae51f7dd63a9bee545150a85f42274a5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 0712e2c9246ba7fe0c81d42861628c60eee2cfa0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 749c2ce8f89fe5960af5a4b48ff45a38091350f4 > > Diff: https://reviews.apache.org/r/55904/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate empty objects that are immediately returned
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56356/#review164560 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 7, 2017, 2:14 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56356/ > --- > > (Updated Feb. 7, 2017, 2:14 a.m.) > > > Review request for sentry, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1615 > https://issues.apache.org/jira/browse/SENTRY-1615 > > > Repository: sentry > > > Description > --- > > SENTRY-1615 SentryStore should not allocate empty objects that are > immediately returned > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 145808c04c1564eee624f3da2276852e26342c9f > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 321c094366caa2b4a56758781e5fc5a2fc9218d0 > > Diff: https://reviews.apache.org/r/56356/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 56411: SENTRY-1624 DefaultSentryValidator doesn't correctly construct SentryOnFailureHookContextImpl
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56411/#review164752 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 8, 2017, 5:15 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56411/ > --- > > (Updated Feb. 8, 2017, 5:15 a.m.) > > > Review request for sentry, Aihua Xu, Hao Hao, kalyan kumar kalvagadda, > Nachiket Vaidya, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1624 > https://issues.apache.org/jira/browse/SENTRY-1624 > > > Repository: sentry > > > Description > --- > > SENTRY-1624 DefaultSentryValidator doesn't correctly construct > SentryOnFailureHookContextImpl > > > Diffs > - > > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java > c9da3ab0a901095acc0d78ce2ed0da0f0038ee56 > > Diff: https://reviews.apache.org/r/56411/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 56454: SENTRY-1609: DelegateSentryStore is subject to JDQL injection
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56454/#review164753 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 8, 2017, 5:34 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56454/ > --- > > (Updated Feb. 8, 2017, 5:34 p.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1609 > https://issues.apache.org/jira/browse/SENTRY-1609 > > > Repository: sentry > > > Description > --- > > SENTRY-1609: DelegateSentryStore is subject to JDQL injection > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > f65c83fc8beeb13850bff94409a679be3ce88353 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 2e63d05edf302052b11ddafab998328c6571eb33 > > Diff: https://reviews.apache.org/r/56454/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 56861: SENTRY-1636 Remove thrift dependency on fb303
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56861/#review166176 --- Ship it! Ship It! - Vamsee Yarlagadda On Feb. 21, 2017, 2:34 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56861/ > --- > > (Updated Feb. 21, 2017, 2:34 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1636 > https://issues.apache.org/jira/browse/SENTRY-1636 > > > Repository: sentry > > > Description > --- > > SENTRY-1636 Remove thrift dependency on fb303 > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift > 5f9cf3137465f08cde99df146e643b1762f695dc > sentry-hdfs/sentry-hdfs-service/pom.xml > b890dd5647e4556d3d0e9dfb54b3b02bc8683359 > sentry-service/sentry-service-common/pom.xml > bac8c650a3e28821df4abcbda15035b867218b5a > > sentry-service/sentry-service-common/src/main/resources/sentry_common_service.thrift > 65c6934bc57b5b979c1d757b51f6785178398bd0 > > sentry-service/sentry-service-common/src/main/resources/sentry_generic_policy_service.thrift > db107bfdeaf6a47f599f7d779c91396ecc53ee7a > > sentry-service/sentry-service-common/src/main/resources/sentry_policy_service.thrift > 82cd947f3d5a077a0f9aab553a7c65c6efa05228 > sentry-service/sentry-service-server/pom.xml > e8098be4476684e84fc34c81dbb7203b9e1666ea > > sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TomcatSqoopRunner.java > cea9acc58fb54aed353b91537f5a9c70326f1d29 > > Diff: https://reviews.apache.org/r/56861/diff/ > > > Testing > --- > > I re-generated all Thrift files to verify that there are no actual changes in > the generated code. > > > Thanks, > > Alexander Kolbasov > >
Review Request 56892: SENTRY-1635: Limit HMS connections only to the leader of the sentry servers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56892/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0 Diff: https://reviews.apache.org/r/56892/diff/ Testing --- Thanks, Vamsee Yarlagadda
Re: Review Request 56134: SENTRY-1593 Implementing client failover for Generic policy clients and namenode clients
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review166181 --- sentry-core/sentry-core-common/pom.xml (lines 65 - 68) <https://reviews.apache.org/r/56134/#comment238059> Do we need this dependency? Sasha put out a code review to take this out. https://reviews.apache.org/r/56861/ - Vamsee Yarlagadda On Feb. 10, 2017, 9:44 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56134/ > --- > > (Updated Feb. 10, 2017, 9:44 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > There are couple of things done as part of this change set > 1. Extended the capability of sentry Namenode client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Made design change so that logic that handles the transport layer of the > client to another class so that we don't have to duplicate the code and also > more maintainable. > 3. Moved the service constants need to handle the transport of these clients > separately. > > Note: Plese refer jira to look at the class diagram attached. > > > Diffs > - > > sentry-core/sentry-core-common/pom.xml 9d18063 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > d321531 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/ThriftUtil.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/PolicyServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 2a18b15 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > db55b5a > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java > eccf83b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java > d320d0f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java > 11cdee7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > ee6cdf7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java > 980d930 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java > f6bb8a5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 1e72b74 > > se
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to the leader of the sentry servers
> On Feb. 22, 2017, 1:48 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 284 > > <https://reviews.apache.org/r/56892/diff/1/?file=1640969#file1640969line284> > > > > If close() generates exception, we don't sent client to null and don't > > clean kerberos context. > > Can this be refactored so that we always set client to null and > > kerberos context to null even if we get exception? We may I don't want to > > have closed client or kereros context lying around. After checking the code of client.close(), I noticed that it cannot throw an exception to the user as all the associated exceptions were being caught in the method definition itself. Anyway I refactored the code to set client, kerberosContext to null irrespective of the error it encounters. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56892/#review166290 ------- On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56892/ > --- > > (Updated Feb. 21, 2017, 4:03 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Looking at the code of HMSFollower, we seem to be establishing HMS connection > for all servers of Sentry servers irrespective of whether they are leader or > not. Whereas the subsequent operations of HMSFollower are only limited to the > leader of the Sentry group. To avoid extra connections, we can limit only the > leader to get a connection to HMS. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0 > > Diff: https://reviews.apache.org/r/56892/diff/ > > > Testing > --- > > > Thanks, > > Vamsee Yarlagadda > >
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to the leader of the sentry servers
> On Feb. 22, 2017, 1:48 a.m., Alexander Kolbasov wrote: > > Posted a new review for these changes: https://reviews.apache.org/r/56931/ - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56892/#review166290 --- On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56892/ > --- > > (Updated Feb. 21, 2017, 4:03 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > --- > > Looking at the code of HMSFollower, we seem to be establishing HMS connection > for all servers of Sentry servers irrespective of whether they are leader or > not. Whereas the subsequent operations of HMSFollower are only limited to the > leader of the Sentry group. To avoid extra connections, we can limit only the > leader to get a connection to HMS. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0 > > Diff: https://reviews.apache.org/r/56892/diff/ > > > Testing > --- > > > Thanks, > > Vamsee Yarlagadda > >
Review Request 56931: [2/2] SENTRY-1635: Limit HMS connections only to the leader of the sentry servers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56931/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Addressing the review comments made my Sasha in the code review: https://reviews.apache.org/r/56892/ Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1a99a045cdcb7225ff29b379497ddbc8e5ec4c2b Diff: https://reviews.apache.org/r/56931/diff/ Testing --- Thanks, Vamsee Yarlagadda
Review Request 57308: SENTRY-1388: Make HiveConf and Hive client jars available to Sentry deamon
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57308/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Hive client jars will automatically be put under the classpath of Sentry process by Maven. For hive-conf, one has to set a standard environmental variable for defining Hive client configs - HIVE_CONF_DIR. In order for sentry to pick this up, this patch will make sentry scripts be aware of this variable. Diffs - bin/sentry 93809eaa1beb617d6977966e4ffa1d1262e8d3d9 Diff: https://reviews.apache.org/r/57308/diff/1/ Testing --- Trivial but will test it out on real cluster before poushing in Thanks, Vamsee Yarlagadda
Re: Review Request 57308: SENTRY-1388: Make HiveConf and Hive client jars available to Sentry deamon
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57308/ --- (Updated March 3, 2017, 10:41 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- Hive client jars will automatically be put under the classpath of Sentry process by Maven. For hive-conf, one has to set a standard environmental variable for defining Hive client configs - HIVE_CONF_DIR. In order for sentry to pick this up, this patch will make sentry scripts be aware of this variable. Diffs - bin/sentry 93809eaa1beb617d6977966e4ffa1d1262e8d3d9 Diff: https://reviews.apache.org/r/57308/diff/1/ Testing (updated) --- Tested on the cluster before and after the patch and verified the client configs are being passed correctly in a way that HiveConf instantiation is automatically picking up. (Screenshot attached) File Attachments (updated) Configs being picked up https://reviews.apache.org/media/uploaded/files/2017/03/03/34145b57-cb71-4816-aa8d-3b669f90ec87__Screen_Shot_2017-03-04_at_4.09.56_AM.png Thanks, Vamsee Yarlagadda
Re: Review Request 57308: SENTRY-1388: Make HiveConf and Hive client jars available to Sentry deamon
> On March 4, 2017, 12:55 a.m., Alexander Kolbasov wrote:
> > bin/sentry
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/57308/diff/1/?file=1655752#file1655752line78>
> >
> > Do you need to check that HIVE_CONF_DIR is set?
> > Something like
> >
> > [[ -n $HIVE_CONF_DIR ]] && HADOOP_CLASSPATH+=":${HIVE_CONF_DIR}"
I thought about this, but it shouldn't hurt the definition of classpath even
though if it is not set/empty. So I went with something easier to read (code
wise).
- Vamsee
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57308/#review167913
-------
On March 3, 2017, 10:41 p.m., Vamsee Yarlagadda wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57308/
> ---
>
> (Updated March 3, 2017, 10:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar
> kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> Hive client jars will automatically be put under the classpath of Sentry
> process by Maven. For hive-conf, one has to set a standard environmental
> variable for defining Hive client configs - HIVE_CONF_DIR. In order for
> sentry to pick this up, this patch will make sentry scripts be aware of this
> variable.
>
>
> Diffs
> -
>
> bin/sentry 93809eaa1beb617d6977966e4ffa1d1262e8d3d9
>
>
> Diff: https://reviews.apache.org/r/57308/diff/1/
>
>
> Testing
> ---
>
> Tested on the cluster before and after the patch and verified the client
> configs are being passed correctly in a way that HiveConf instantiation is
> automatically picking up.
>
> (Screenshot attached)
>
>
> File Attachments
>
>
> Configs being picked up
>
> https://reviews.apache.org/media/uploaded/files/2017/03/03/34145b57-cb71-4816-aa8d-3b669f90ec87__Screen_Shot_2017-03-04_at_4.09.56_AM.png
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: [DISCUSS] Merging Sentry HA branch with master
> > Cherry-pick any commits that happened since sentry-ha-redesign was forked, > except a few described below > Exclude big refactoring commit (SENTRY-1205) and related commits > (SENTRY-1436, SENTRY-1438, SENTRY-1406) > Rename master to a dev branch > Rename sentry-ha-redesign to master This sounds good to me. Generally having merge commits complicates the git history and might get tricky when we are debugging things. I would rather stick with the approach of cherry-picks to make the history clear. Thanks, Vamsee On Tue, Apr 4, 2017 at 7:17 PM, Alexander Kolbasov wrote: > I would like to make a more concrete proposal and I am interested in > opinion from Sentry PMC members on this. > > I would propose the following approach for merging Sentry HA into Sentry > master: > > Cherry-pick any commits that happened since sentry-ha-redesign was forked, > except a few described below > Exclude big refactoring commit (SENTRY-1205) and related commits > (SENTRY-1436, SENTRY-1438, SENTRY-1406) > Rename master to a dev branch > Rename sentry-ha-redesign to master > > What does community think about such approach? > > - Alex > > > > On Mar 22, 2017, at 1:43 PM, Alexander Kolbasov > wrote: > > > > Hello, > > > > I would like to start the discussion on merging sentry-ha-redesign > branch with master. > > > > As of now most of the changes from master are merged into > sentry-ha-redesign. The major missing part is SENTRY-1205 (Refactor the > code for sentry-provider-db and create sentry-service module) and > associated issues. This refactoring is very hard to port, especially since > there is very little information in the JIRA on why it was done and how it > was done - was it merely moving files around or more then that. I would > seriously consider not including this change in 1.8. > > > > So in regards to the merge we have several options: > > > > Attempt to merge master into sentry-ha-redesign, resolve any conflicts > and later commit the merge to master. This will cause merge commit on master > > Finish work on sentry-ha-redesign, make sure that relevant commits are > ported from master, and then making this a master branch and making current > master a special branch left for reference purposes. This will likely leave > SENTRY-1205 and related issues out. > > What does community think about this? > > > > - Alex > > -- Thanks, Vamsee
Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review172262 --- Fix it, then Ship it! LGTM Otherwise sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 275 (patched) <https://reviews.apache.org/r/58221/#comment245364> I know it doesn't hurt to check for hmsFollower != null but to make the code more readable can we replace this constraint with (notificationLogEnabled == true)? - Vamsee Yarlagadda On April 18, 2017, 3:46 a.m., Na Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > --- > > (Updated April 18, 2017, 3:46 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/15/ > > > Testing > --- > > > Thanks, > > Na Li > >
Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58284/#review172277 --- Ship it! LGTM - Vamsee Yarlagadda On April 18, 2017, 12:26 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58284/ > --- > > (Updated April 18, 2017, 12:26 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio > Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1687 > https://issues.apache.org/jira/browse/SENTRY-1687 > > > Repository: sentry > > > Description > --- > > SENTRY-1687 FullUpdateInitializer can be more efficient > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > 90aaaef0e15306627d7108f12a74a29848055c0b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java > f338ce8abddcde128536a0efef77983990325aa6 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java > b5cbea9d295385bb38b912c13903edace04d7589 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java > e643e01a45de77ef7f465d6921c5ae9e7ce925a2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb13b0d5015aefe892a6f7e46812ea75124 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 > > > Diff: https://reviews.apache.org/r/58284/diff/2/ > > > Testing > --- > > Updated the unit test to test for bigger HMS snapshots > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients
> On March 31, 2017, 6:18 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java > > Lines 33 (patched) > > <https://reviews.apache.org/r/57901/diff/2/?file=1680343#file1680343line33> > > > > I think this should be AutoCloseable > > kalyan kumar kalvagadda wrote: > Why not Closable? Comment from http://stackoverflow.com/questions/13141302/implements-closeable-or-implements-autocloseable. Seems like a very usuable thing for clients. Implementing AutoCloseable allows a class to be used as a resource of the try-with-resources construct introduced in Java 7, which allows closing such resources automatically at the end of a block, without having to add a finally block which closes the resource explicitely. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57901/#review170671 --- On April 18, 2017, 11:18 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57901/ > --- > > (Updated April 18, 2017, 11:18 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1593 > https://issues.apache.org/jira/browse/SENTRY-1593 > > > Repository: sentry > > > Description > --- > > At a high level the change set submitted does following. > 1. Extends the capability of sentry name-node client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Transport handling of the clients is abstracted to another class so that > the client's just have to extend the class that abstracts it. Implementations > of the clients don't have to worry about it. > 3. Changed the behavior of the clients so that they connect to one of the > configured servers when the client is actually instantiated. > > As part of the code changes there are couple of files that are moved from one > package to another. Here is the list of them > 1. RetryClientInvocationHandler.java > 2. SentryClientInvocationHandler.java > 3. ThriftUtil.java > 4. SentryHdfsServiceException.java > Amount these files, RetryClientInvocationHandler.java has changed. Please > have a closer look at this file. > > > Diffs > - > > sentry-core/sentry-core-common/pom.xml > 9d180635ca5ed6fb1def4ffdc4addda5a650593e > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > 4ed1361fb1d79620c3098355a0ea9c27203a75de > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > sentry-hdfs/sentry-hdfs-dist/pom.xml > beda2029616538101dc368b3d272fdfc942868ad > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 085971b34e3901b7a1d59bd8e7516b25f81ca872 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf402a9a04b28e2c6c06d4e55a3607132ead > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e1bed10dcd706ef12c078305b6497874f4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFac
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58412/#review172289 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2728-2731 (patched) <https://reviews.apache.org/r/58412/#comment245394> If the mOldPaths is empty, we simply log the error and not do anything. Do you think we should add the new path (mNewPath) irrespective of oldPath existed or not? - Vamsee Yarlagadda On April 18, 2017, 9:34 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58412/ > --- > > (Updated April 18, 2017, 9:34 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > --- > > SENTRY-1587: Refactor SentryStore transaction to persist a single path > transcation bundled with corresponding delta path change > > Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > 90aaaef0e15306627d7108f12a74a29848055c0b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > c22364fa26c80415827313f4d26f6c53b71b6f6c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java > c74384688ca920c79fb2987d225042e135cdfd53 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 08520f30f3529af88d7dab9ae3623f28f0e43558 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb13b0d5015aefe892a6f7e46812ea75124 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 87e329588990be129197d598dd1db4487f8e0f25 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 7769f24bb4c062016084bcafe7d50a0f0e013824 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 0e97466c9ba6973d8e787819e292c3b826472b38 > > > Diff: https://reviews.apache.org/r/58412/diff/3/ > > > Testing > --- > > 1. Add new unit test in TestSentryStore > 2. Enabled hdfs sync integration test > > > Thanks, > > Hao Hao > >
Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57901/#review172294 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java Line 44 (original), 43 (patched) <https://reviews.apache.org/r/57901/#comment245398> nit. Typo at invocation - Vamsee Yarlagadda On April 18, 2017, 11:18 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57901/ > --- > > (Updated April 18, 2017, 11:18 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1593 > https://issues.apache.org/jira/browse/SENTRY-1593 > > > Repository: sentry > > > Description > --- > > At a high level the change set submitted does following. > 1. Extends the capability of sentry name-node client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Transport handling of the clients is abstracted to another class so that > the client's just have to extend the class that abstracts it. Implementations > of the clients don't have to worry about it. > 3. Changed the behavior of the clients so that they connect to one of the > configured servers when the client is actually instantiated. > > As part of the code changes there are couple of files that are moved from one > package to another. Here is the list of them > 1. RetryClientInvocationHandler.java > 2. SentryClientInvocationHandler.java > 3. ThriftUtil.java > 4. SentryHdfsServiceException.java > Amount these files, RetryClientInvocationHandler.java has changed. Please > have a closer look at this file. > > > Diffs > - > > sentry-core/sentry-core-common/pom.xml > 9d180635ca5ed6fb1def4ffdc4addda5a650593e > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > 4ed1361fb1d79620c3098355a0ea9c27203a75de > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > sentry-hdfs/sentry-hdfs-dist/pom.xml > beda2029616538101dc368b3d272fdfc942868ad > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 085971b34e3901b7a1d59bd8e7516b25f81ca872 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf402a9a04b28e2c6c06d4e55a3607132ead > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e1bed10dcd706ef12c078305b6497874f4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 2a18b1524546840df9e3275bd8a8220cc9de1b1f > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > 4dc99a2bb6ec748a42ae2b65e937f6dec492fc48 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3172533fa768145d23664ab536dcadd6b3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java > 919b81bd657e00841f4c8bb63a8851d1a44f8b78 > > sentry-provider/sentry-
Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer
> On April 18, 2017, 9:15 p.m., Vamsee Yarlagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 275 (patched) > > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line307> > > > > I know it doesn't hurt to check for hmsFollower != null but to make the > > code more readable can we replace this constraint with > > (notificationLogEnabled == true)? > > Na Li wrote: > check notification itself is not enough. If hmsFollower is null, we don't > want to continue scheduling its task. > > I was checking both notifcation and hmsFollower. As you mentioned, > checking notificationLogEnabled makes code more readable. But Kalyan wants to > get rid of it to make the code simplier. > > So how can I satisfy both preference? Or you two can reach agreement? > > Na Li wrote: > Sasha mentioned that notificationLogEnabled will be gone soon as it will > always be true. If it is false, Hive and Sentry won't work together. The reason why I left my comment was - Looking at the code, I realized that hmsFollower can never be null if the constructor of SentryService passed. So i thought it is not really necessarily to check whether it is null or not. But looking at your comment pointing to Sasha's note that this notificationLogEnabled flag will soon go away, then we can leave it as is to avoid making changes again. - Vamsee --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review172262 --- On April 19, 2017, 2:23 p.m., Na Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > --- > > (Updated April 19, 2017, 2:23 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/17/ > > > Testing > --- > > > Thanks, > > Na Li > >
Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review172434 --- LGTM otherwise. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 285 (original), 291 (patched) <https://reviews.apache.org/r/58221/#comment245557> nit. Could not start the "periodic executor of" HMSFollower. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 312 (original), 320 (patched) <https://reviews.apache.org/r/58221/#comment245558> This is getting trickier. Don't we have to call awaitTermination() after this? - Vamsee Yarlagadda On April 20, 2017, 12:17 a.m., Na Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > --- > > (Updated April 20, 2017, 12:17 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/18/ > > > Testing > --- > > > Thanks, > > Na Li > >
Re: Review Request 58782: SENTRY-1725 Include response status in TSentrySyncIDResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58782/#review173246 --- Ship it! Ship It! - Vamsee Yarlagadda On April 27, 2017, 7:08 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58782/ > --- > > (Updated April 27, 2017, 7:08 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio > Pena, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1725 > https://issues.apache.org/jira/browse/SENTRY-1725 > > > Repository: sentry > > > Description > --- > > SENTRY-1725 Include response status in TSentrySyncIDResponse > > > Diffs > - > > pom.xml c29dc2efc0f297dd2ee3b8c4996f09f41038dbe0 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/SentryHDFSService.java > 5cbd26ce71e215ce4026850c1599ab051aa47b40 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateResponse.java > c21072f2c6326c669f878443817bb14686561eb7 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathChanges.java > 4ba4f2754243f47457bbc122fe00d2337ca317ba > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java > 722eaeca288c8d62acd06f246c53c482ab690665 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsDump.java > c1b42a0c3e4887e2bcc1567403575525da91e481 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsUpdate.java > f469d4a662ad2a0e84d104bc29d9bdeba630d967 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPermissionsUpdate.java > 727b7053c74420bfcedd368b6bb2e8607cbc9211 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java > 277118dfdd188ab07df7a9231458d3ce9d0760f1 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TRoleChanges.java > 879f13e155992f12d61ff76f64bf26546bcf65f9 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyService.java > aa56e8d98e31f92a7986ab5c2bb237188de10a6f > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsRequest.java > 7e3a14c01daa49434302e8c8ec2ff9823163a64b > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsResponse.java > 47555a6ca047cb6978afa91277eec313545e3349 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsRequest.java > a754fe46a83cfe46b422326204987501fa668b3e > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsResponse.java > ded6d2434a1e4bd8fcdb69821b7edbb3078cc0f3 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeRequest.java > d7b65fd5e108198fb0f80613798781f90927e6f9 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeResponse.java > fcb1db35aa7c927dc17e3c1a32a8ac4f8d1eaa5e > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleRevokePrivilegeRequest.java > de0b371b473a574e742bd7f03438bda7580e6251 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleRevokePrivilegeResponse.java > d37fe4efc0a0114d7205f4860cf39ddd73daa9a1 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAuthorizable.java > e0af0458613c4eb2553dec789a5644cd60f63ebf > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TCrea
Re: Review Request 58808: SENTRY-1726 sql changes to store last notification-id processed
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58808/#review173249 --- Fix it, then Ship it! LGTM otherwise sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql Lines 24-34 (patched) <https://reviews.apache.org/r/58808/#comment246292> Not sure what was the rule behind creating these patch files like SENTRY-1726.derby.sql for this change? - Vamsee Yarlagadda On April 27, 2017, 6:19 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58808/ > --- > > (Updated April 27, 2017, 6:19 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1726 > https://issues.apache.org/jira/browse/SENTRY-1726 > > > Repository: sentry > > > Description > --- > > 1. Created new table to store the last notification id > 2. Table is defined to store only one entry. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql > ba70715 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql > 879e732 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql > e83ab83 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql > c28099b > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql > 841dcaa > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql > 223835e > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql > 006d57b > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql > 20921ea > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql > 489ad66 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql > ec50912 > > > Diff: https://reviews.apache.org/r/58808/diff/1/ > > > Testing > --- > > > Thanks, > > kalyan kumar kalvagadda > >
Review Request 58913: SENTRY-1745: Bundle sentry-core-common into sentry-hdfs-dist to avoid NN failing with NoClassDefFoundError
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58913/ --- Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda. Repository: sentry Description --- With the recent refactoring change SENTRY-1639, we introduced a new class SentryClientTransportConfigInterface. While trying to spin up a real cluster with this change, NN could fail to start with NoClassDefFoundError as we only put the jars under SENTRY_HOME/lib/plugins on the HDFS classpath and this new class is part of sentry-core-common which is not bundled inside plugins/. Diffs - sentry-hdfs/sentry-hdfs-dist/pom.xml dd2d847 Diff: https://reviews.apache.org/r/58913/diff/1/ Testing --- Verified to see the sentry-core-common being bundled. Thanks, Vamsee Yarlagadda
Re: Review Request 58926: SENTRY-1747: HMSFollower shouldn't create local hive during tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58926/#review173632 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 132 (patched) <https://reviews.apache.org/r/58926/#comment246632> Do you think this is something we need to raise an error even outside of tests? - Vamsee Yarlagadda On May 2, 2017, 5:44 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58926/ > --- > > (Updated May 2, 2017, 5:44 p.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and > Vamsee Yarlagadda. > > > Bugs: SENTRY-1747 > https://issues.apache.org/jira/browse/SENTRY-1747 > > > Repository: sentry > > > Description > --- > > SENTRY-1747: HMSFollower shouldn't create local hive during tests > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > ec8676e5c4e4030b4a8d819bec028694b96fd189 > > > Diff: https://reviews.apache.org/r/58926/diff/1/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Review Request 59006: SENTRY-1749: Sentry fails to setup secure connection to HMS if the service unix user is missing active tgt
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59006/ --- Review request for sentry and Alexander Kolbasov. Repository: sentry Description --- Looking at the code of Sentry and HiveMetaStoreClient, it looks like this problem has been there for a long time. But it's mostly masked by the restarts we are doing after the clean deployment. So during the course of initial service start, sentry and other services don't have any keytabs active under local unix user and during this time, the log of sentry is cluttered with the above errors. But after we do a restart after deployment completes, the Sentry code picks up the principal that was activated under "sentry" user (which users do it for testing purposes) and thus works properly. *Ideally* the service processes shouldn't depend on the keytabs active on running unix user but rather use the keytabs supplied by startup system in the process directory. Looking at the code of [HMSFollower|https://github.com/apache/sentry/blob/sentry-ha-redesign/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java] we invoke the classes of Hive - HiveMetaStoreClient which exclusively uses the UserGroupInformation object. And for UserGroupInformation object to pick up the keytab, one should explicitly call the methods like [loginUserFromKeytabAndReturnUGI|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L1406] or use [getUGIFromSubject|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L841]. For the fact that HMSFollower already logs into the keytab, we can leverage the method getUGIFromSubject() to make the UserGroupInformation aware of keytab a uthentication. If UGI object is not made aware of keytab, then the invocation of [UserGroupInformation#getCurrentUser|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L736-L742] falls back to use Unix user. To avoid this, we need to add extra logic to HMSFollower to make UserGroupInformation aware of the keytab that way it uses it for communication with HMS. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e Diff: https://reviews.apache.org/r/59006/diff/1/ Testing --- Successfully verified that without keytab active on the unix user, the program was successfully able to connect to HMS using the supplied keytab. Thanks, Vamsee Yarlagadda
Re: Review Request 59132: SENTRY-1759 UpdatableCache leaks connections
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59132/#review174495 --- Ship it! Same comment as Sergio's. Using "try-with-resources" seems like a good idea. - Vamsee Yarlagadda On May 10, 2017, 7:48 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59132/ > --- > > (Updated May 10, 2017, 7:48 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1759 > https://issues.apache.org/jira/browse/SENTRY-1759 > > > Repository: sentry > > > Description > --- > > SENTRY-1759 UpdatableCache leaks connections > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > f272630171763b8afa2cbd826cb2004532e52a4a > > > Diff: https://reviews.apache.org/r/59132/diff/1/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 59120: SENTRY-1757: Avoid using local hive meta store using wrong configuration
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59120/#review174559 --- Ship it! Ship It! - Vamsee Yarlagadda On May 10, 2017, 6:44 p.m., Na Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59120/ > --- > > (Updated May 10, 2017, 6:44 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Sergio Pena. > > > Bugs: sentry-1757 > https://issues.apache.org/jira/browse/sentry-1757 > > > Repository: sentry > > > Description > --- > > use hive configuration to check. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > e271ea4 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 1530eb2 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 32acc65 > > > Diff: https://reviews.apache.org/r/59120/diff/3/ > > > Testing > --- > > integration test TestHDFSIntegrationEnd2End > > > Thanks, > > Na Li > >
Re: [DISCUSS] Having an interactive chat account for Apache Sentry
+1. IIRC, they are using slack. On Thu, May 11, 2017 at 4:21 PM, Kalyan Kumar Kalvagadda < kkal...@cloudera.com> wrote: > Sasha, > > That an nice idea. Do you know what Apache infrastructure is kudu project > using? > > -Kalyan > > -Kalyan > > On Thu, May 11, 2017 at 6:12 PM, Alex Kolbasov wrote: > > > What do people think about having Apache Sentry chat room where we can > > hang out and informally chat and discuss technical and non-technical > > issues? Is there any Apache infrastructure for that (e.g. HipChat or > Slack > > account)? If not we can use some private account for that purpose (I > think > > that’s what Kudu is doing). > > > > Any thoughts about this? > > > > - Alex > -- Thanks, Vamsee
Re: Review Request 58808: SENTRY-1726 sql changes to store last notification-id processed
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58808/#review174750 --- Ship it! Ship It! - Vamsee Yarlagadda On May 9, 2017, 11:15 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58808/ > --- > > (Updated May 9, 2017, 11:15 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1726 > https://issues.apache.org/jira/browse/SENTRY-1726 > > > Repository: sentry > > > Description > --- > > 1. Created new table to store the last notification id > 2. Table will store the notification id's of all the notifications processed > by sentry server. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql > ba70715 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql > 879e732 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql > e83ab83 > > sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql > c28099b > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql > 841dcaa > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql > 223835e > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql > 006d57b > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql > 20921ea > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql > 489ad66 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql > ec50912 > > > Diff: https://reviews.apache.org/r/58808/diff/2/ > > > Testing > --- > > Tested with the databases that sentry supports. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 59167: SENTRY-1580: Provide pooled client connection model with HA
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59167/#review174745 --- sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java Line 362 (original), 362 (patched) <https://reviews.apache.org/r/59167/#comment247960> I wonder if this goes into the process log or the stdout/stderr? But better to use LOGGER i guess. And in addition to that, this will swallow the exception and not raise it. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java Lines 374 (patched) <https://reviews.apache.org/r/59167/#comment247962> Same comment as above. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java Line 99 (original), 110 (patched) <https://reviews.apache.org/r/59167/#comment247977> Does this connection pooling create and keep the connections on demand basis? Or it tries to establish this number of connections before proceeding further? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java Line 288 (original), 133 (patched) <https://reviews.apache.org/r/59167/#comment247980> nit. Can we have this message before returning from this method that a secure connection was established? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java Lines 155 (patched) <https://reviews.apache.org/r/59167/#comment247979> Even though the rest of this class is ThreadSafe, this invocation of USerGroupInformation is not. If there was a case where there are differences in the conf supplied, this could result in a strange behavior. But it is highly unlikely that conf/ changes within the running process. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 88 (patched) <https://reviews.apache.org/r/59167/#comment247981> Just wondering, is this thread safe? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 94 (patched) <https://reviews.apache.org/r/59167/#comment247982> Shouldn't this come at the end of this constructor? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 160 (patched) <https://reviews.apache.org/r/59167/#comment247985> Will commons-pool ensure that the object being returned has valid active connection? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 199-200 (patched) <https://reviews.apache.org/r/59167/#comment247984> This message could be a bit confusing if isPoolEnabled is set to false sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 250 (patched) <https://reviews.apache.org/r/59167/#comment247986> nit. typo sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java Line 30 (original), 31 (patched) <https://reviews.apache.org/r/59167/#comment247987> ThreadSafe annotation? - Vamsee Yarlagadda On May 11, 2017, 7:17 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59167/ > --- > > (Updated May 11, 2017, 7:17 a.m.) > > > Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na > Li, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1580 > https://issues.apache.org/jira/browse/SENTRY-1580 > > > Repository: sentry > > > Description > --- > > SENTRY-1580: Provide pooled client connection model with HA > > > Diffs > - > > pom.xml ad54cfda5e7dabf9ecadc8f8d9f0e3012596fa32 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > a3140f2e23188e0bc7b6d5521a2f457d090a937f > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java > 262db11677c1bb999265a5033971c06def9455ed > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > da587d00aa6e4b2fb6d78e3a720464d25bdcb47c > > sentry-binding/sentry-binding-hive/src/main/java
Re: Review Request 59235: SENTRY-1765 CounterWait.update throw exception when input is old
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59235/#review174849 --- Ship it! Ship It! - Vamsee Yarlagadda On May 12, 2017, 5:45 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59235/ > --- > > (Updated May 12, 2017, 5:45 p.m.) > > > Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na > Li, Sergio Pena, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1765 > https://issues.apache.org/jira/browse/SENTRY-1765 > > > Repository: sentry > > > Description > --- > > SENTRY-1765 CounterWait.update throw exception when input is old > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java > 2b4ee8411a9d0e170cac44bd0cf9a560297412f2 > > > Diff: https://reviews.apache.org/r/59235/diff/1/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
