Broken e2e tests in master?

[Alexander Kolbasov]

I was looking at test failures from my patch and the failures seem to be
quite unrelated. A lot of e2e tests failed because there were too many
failures like

Unable to finalize edits file
/tmp/1477513681071-0/dfs/name2/current/edits_inprogress_001

For example here you can see these failures:

https://builds.apache.org/job/PreCommit-SENTRY-Build/2100/testReport/

Can we get a clean run of e2e tests in master now?

- Alex

Review Request 52916: SENTRY-1505 CommitContext isn't used by anything and should be reoved

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52916/
---

Review request for sentry, Hao Hao, Li Li, and Sravya Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1505 CommitContext isn't used by anything and should be reoved


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
 c23042d06391693f07f6d2da8e941dc68b7b5d3f 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
 c003965af52aa61aea555ef843221fb712ff2f48 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandler.java
 e0a5f03d5cc72dadbd0325efd7e9734a4bf48558 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandlerInvoker.java
 1d9c246214e50cea08b8c69df9a1c6a3080346ed 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
 e59d12a6188d84d1a6bd627dd7776cefa0fbea44 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CommitContext.java
 c74dbf3be18c68aa8fdaca2ce12c24e7e8ae5a54 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 3adf273a164599c0d09f0803a574ee6344fbd4df 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandler.java
 b1a4b7fb39af3db1181a125365da09655694c480 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandlerInvoker.java
 856ef9a6a830ab6829d55cfe45a8e7f15620b81b 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 5dff12a5026ebb6764e83baf90c7b12d069701fa 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
 5ada04c3d03e7327ae0e23d4a23e346a8e4e25a8 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java
 8b3599fd9cb4dd80700c6bf405d35db16a72e3d7 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 600e1185a4d86be56e49f5f91873cb847cf53980 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestNotificationHandlerInvoker.java
 6a2f48f39246efbb5d7e80b4562ea19240dfd8b6 

Diff: https://reviews.apache.org/r/52916/diff/


Testing
---


Thanks,

Alexander Kolbasov

Question about SentryStore/DelegateSentryStore

[Alexander Kolbasov]

There is some strangeness in the way SentryStore/DelegateSentryStore is
implemented.

DelegateSentryStore implements SentryStoreLayer interface, but SentryStore
doesn't. It looks like SentryStore should implement this as well.

Any thoughts on this?

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52138/#review152450
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 237)
<https://reviews.apache.org/r/52138/#comment221461>

These two error messages are always indistinguishable. Can you reword them 
so that it is easy to see whether we got an exception or ID changed? It would 
be nice to be able to grep for specific word to see whether any of these 
happened.


- Alexander Kolbasov


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52138/#review152452
---


Ship it!




Just one small nit with logging.

- Alexander Kolbasov


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52888: NPE in log4j.properties parsing

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52888/#review152713
---


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 14, 2016, 4:08 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52888/
> ---
> 
> (Updated Oct. 14, 2016, 4:08 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1504
> https://issues.apache.org/jira/browse/SENTRY-1504
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There is a NPE if the log4j.properties configuration file does not contain 
> "log.threshold".
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  d321531 
> 
> Diff: https://reviews.apache.org/r/52888/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 51848: Apply Checkstyle changes to the core

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51848/#review152947
---



My big concern with these changes is that they cause more deviation between 
trunk and sentry-ha branch where there are a lot of changes happening. The 
style changes are very useful but they cause quite complicated merges. I think 
that we should consider delaying these changes until we merge sentry HA branch 
into trunk.

- Alexander Kolbasov


On Sept. 13, 2016, 3:57 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51848/
> ---
> 
> (Updated Sept. 13, 2016, 3:57 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1470
> https://issues.apache.org/jira/browse/SENTRY-1470
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This task is to apply checkstyle fixes to the core. Once all modules are 
> covered, the rules can be enabled by default.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaPrivilegeValidator.java
>  ba66d43 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
>  3bc5b82 
>   
> 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/SentryVersionInfo.java
>  de77dc3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/BitFieldAction.java
>  0f5b23b 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Resource.java
>  3ce52e8 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryUserException.java
>  3b5beda 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/AuthorizationComponent.java
>  e3f1f15 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
>  40c9595 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFile.java
>  a6ef0b3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
>  6b625ff 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  4a632bc 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/validator/PrivilegeValidatorContext.java
>  ccee977 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
>  7bc94c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/HiveActionFactory.java
>  ad7e1c9 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/HivePrivilegeModel.java
>  231acca 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java
>  fa28716 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/DatabaseMustMatch.java
>  4276667 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/DatabaseRequiredInPrivilege.java
>  fed3038 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/ServerNameMustMatch.java
>  c79a8bf 
>   
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/ServersAllIsInvalid.java
>  e3f5a3a 
>   
> sentry-core/sentry-core-model-db/src/test/java/org/apache/sentry/core/db/TestURI.java
>  40b60f7 
>   
> sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/IndexerActionFactory.java
>  3ca85bc 
>   
> sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/IndexerModelAuthorizables.java
>  414df68 
>   
> sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/IndexerPrivilegeModel.java
>  6951513 
>   
> sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/validator/AbstractIndexerPrivilegeValidator.java
>  c73fc3c 
>   
> sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/validator/IndexerRequiredInPrivilege.java
>  013c572 
>   
> sent

Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review158578
---



I mean convertToTSentryPrivilege() is similar.

- Alexander Kolbasov


On Dec. 8, 2016, 8:47 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 8, 2016, 8:47 a.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
>  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
> 
>

Re: Review Request 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Alexander Kolbasov]

> 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.

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).


- Alexander


---
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 54526: SENTRY-1540: SentryStore.isMultiActionsSupported() is always true

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158572
---




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/#comment229355>

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?


- Alexander Kolbasov


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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review158576
---



There is also toSentryPrivilege() with similar problem. And once this is fixed 
we don't need fromNULLCol() anymore.

- Alexander Kolbasov


On Dec. 8, 2016, 8:47 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 8, 2016, 8:47 a.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
>  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
> 
>

Re: Review Request 54464: SENTRY-1557: getRolesForGroups(), getRolesForUsers() does too many trips to the the DB

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54464/#review158405
---




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/#comment229171>

It is neat, but does it actually work?


- Alexander Kolbasov


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 54697: SENTRY-1513: Changed the way the split works when parsing a path and corrected spelling in PathsUpdate

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54697/#review159178
---


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 13, 2016, 9:28 p.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54697/
> ---
> 
> (Updated Dec. 13, 2016, 9:28 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1513
> https://issues.apache.org/jira/browse/SENTRY-1513
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Changed the way the split works when parsing a path and corrected spelling in 
> PathsUpdate
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  7cfb3bf 
> 
> Diff: https://reviews.apache.org/r/54697/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/
---

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 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review158278
---




sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java
 (line 16)
<https://reviews.apache.org/r/54454/#comment229050>

This file is auto-generated by Thrift, I don't think we should touch these.



sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
 (line 981)
<https://reviews.apache.org/r/54454/#comment229051>

This is also auto-generated file



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1431)
<https://reviews.apache.org/r/54454/#comment229052>

Why do you want to leave this line here?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1432)
<https://reviews.apache.org/r/54454/#comment229053>

Why privilege.setActionIsSet(false) is called here?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1453)
<https://reviews.apache.org/r/54454/#comment229054>

Why do you want to leave the old code behind?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1720)
<https://reviews.apache.org/r/54454/#comment229056>

MSentryPrivilege is generated by Sentry code itself so it is weird to 
complain about invalid privilege here. I guess the check should be for the 
original privilege instead.

And the message probably should be something like
"grant option is not specified"


- Alexander Kolbasov


On Dec. 6, 2016, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> ---
> 
> (Updated Dec. 6, 2016, 11:05 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 have made changes assuming that grant option is either true/false removing 
> unset. 
> Also, added code so that sentry server could validate the TSentryPrivilege 
> object constructed from the Thrift message received client. If the validation 
> is failed exception is raised and appropriate error is message is sent.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java
>  c056bcc 
>   
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
>  15b339f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  742798d 
> 
> Diff: https://reviews.apache.org/r/54454/diff/
> 
> 
> Testing
> ---
> 
> Verfied the changes using sentry thrift client.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review159250
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 280)
<https://reviews.apache.org/r/54454/#comment230195>

can getPrivileges() return null?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 281)
<https://reviews.apache.org/r/54454/#comment230196>

This is the same code as below - can it be handled by a common func?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 282)
<https://reviews.apache.org/r/54454/#comment230197>

if (!validateGrantOption(request.getPrivilege()) {
  ...
}

There is no point in comparing booleans to true/false



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 283)
<https://reviews.apache.org/r/54454/#comment230198>

can this be moe specific like "grant option is unset" or something like 
this?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 286)
<https://reviews.apache.org/r/54454/#comment230199>

else if {
}



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1066)
<https://reviews.apache.org/r/54454/#comment230194>

It is a bit confusing for these two functions to share the same name.

Also, given that this can be simply written as

return privilege.getGrantOption() != TSentryGrantOption.UNSET

    do we need a function for that at all?


- Alexander Kolbasov


On Dec. 14, 2016, 10:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> ---
> 
> (Updated Dec. 14, 2016, 10:38 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 have made changes assuming that grant option is either true/false removing 
> unset. 
> Also, added code so that sentry server could validate the TSentryPrivilege 
> object constructed from the Thrift message received client. If the validation 
> is failed exception is raised and appropriate error is message is sent.
> 
> 
> 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 54445: SENTRY-1547 - It is possible to create a privilege with all empty fields

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review159252
---



It seems like you have the wrong diff here - you should have changes in 
SentryPolicyStoreProcessor but they are not in this diff.

- Alexander Kolbasov


On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> ---
> 
> (Updated Dec. 14, 2016, 10:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Both the server_name and action feilds are mandatory in previlage request.
> 
> I had to take a different approach in solving the issue based on the review 
> comments. I have added validation logic in SentryPolicyStoreProcessor 
> enforcing the mandatory fields like server_name and action to be present in 
> privilege/privileges added.
> 
> If the validation is failed exception is raised and appropriate error is 
> message is sent.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
>  15b339f 
> 
> Diff: https://reviews.apache.org/r/54445/diff/
> 
> 
> Testing
> ---
> 
> I have done unit tests using sentry cli thrift client.
> 
> 
> File Attachments
> 
> 
> SENTRY-1547.002.patch
>   
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 54409: SENTRY-1476: SentryStore is subject to JDQL injection

[Alexander Kolbasov]

> On Dec. 14, 2016, 9:26 p.m., Vamsee Yarlagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  lines 2647-2654
> > <https://reviews.apache.org/r/54409/diff/1/?file=1577433#file1577433line2647>
> >
> > 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/

I think it is a very good idea, but I'd rather do that as a follow-up fix. The 
goal of this one is to preserve the existing semantics but move all user 
strings to parameters. I'll investigate SENTRY-1577 approach in a later change.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54409/#review159226
-------


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

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
---

(Updated Dec. 14, 2016, 11:57 p.m.)


Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comment for timer stop() being called twice


Repository: sentry


Description
---

SENTRY-1515: Cleanup exception handling in SentryStore


Diffs (updated)
-

  
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
 e8d0e2505c5b178cae3d27e7d85caa652f630a2d 

Diff: https://reviews.apache.org/r/54338/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 54445: SENTRY-1547 - It is possible to create a privilege with all empty fields

[Alexander Kolbasov]

> On Dec. 6, 2016, 10:40 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java,
> >  line 1031
> > <https://reviews.apache.org/r/54445/diff/2/?file=1578156#file1578156line1031>
> >
> > 1. What if it's an empty string (i.e. combination of any space 
> > characterds)? Should probably be treated as missing value?
> > 
> > 2. Should we strip leading and trailing spaces if any?
> 
> kalyan kumar kalvagadda wrote:
> Yeah, that is an invalid input. Should be addressed.

That's a good point - we do this stripping every time in SentryStore code.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54445/#review158238
---


On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54445/
> ---
> 
> (Updated Dec. 14, 2016, 10:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colin Ma, Hao Hao, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Both the server_name and action feilds are mandatory in previlage request.
> 
> I had to take a different approach in solving the issue based on the review 
> comments. I have added validation logic in SentryPolicyStoreProcessor 
> enforcing the mandatory fields like server_name and action to be present in 
> privilege/privileges added.
> 
> If the validation is failed exception is raised and appropriate error is 
> message is sent.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
>  15b339f 
> 
> Diff: https://reviews.apache.org/r/54445/diff/
> 
> 
> Testing
> ---
> 
> I have done unit tests using sentry cli thrift client.
> 
> 
> File Attachments
> 
> 
> SENTRY-1547.002.patch
>   
> https://reviews.apache.org/media/uploaded/files/2016/12/14/88c4a123-316d-4ef4-8670-ad38d260f8cb__SENTRY-1547.002.patch
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: [DISCUSS] Sentry interactive shell

[Alexander Kolbasov]

> On Dec 14, 2016, at 8:46 PM, Lenni Kuff <lsk...@cloudera.com> wrote:
> 
> Thanks for kicking this off Sasha. IMO, a robust Sentry CLI seems super
> important for the project - especially as Sentry support for more
> components comes online. It also helps to allow 3rd party integration with
> Sentry.

I would like to understand this use case a bit better. Do you have some 
specific examples in mind? What I discovered while working on CLI interfaces is 
that the current code assumes that it is dealing with a “well-behaving” client 
where the good behavior isn’t well defined. Just obeying the Thrift protocol is 
triggering various interesting issues (which should be fixed IMO). So for 
applications the API was never thrift but a some Java interfaces. Once you 
start dealing with CLIs (and with 3rd party integrations) it is really 
important to define what exactly is the API - is it defined at the Thrift level 
or at Java interface level.

> Is there a design document / JIRA / other material to review on the new
> CLI?

There isn’t. So far I implemented two Sentry CLIs - one classical command-line 
which is super useful for me (as a developer) and which is written in GO and 
allows easy access to the Thrift protocol itself. Another one (interactive CLI) 
 I wrote mostly to demonstrate client failover for Sentry HA but it is full 
featured (although it only supports Hive model for now)- you can do all CRUD 
operations on roles/privileges/groups and *it goes through the same Java code 
path as all other clients*. My goal was to write the smallest amount of code 
and have a fully working (and useful) version quickly. If there is an interest 
to open this up to a wider audience, we may start a discussion on the design 
choices, e.t.c.

> Given SentryShell is has been in development for this same purpose
> (and has already been released), it would be good to understand the delta
> between the two and motivation for adding a new CLI vs updating
> SentryShell.

I am curious whether anyone has any experience using the SentryShell. I only 
could find this documentation: 
https://cwiki.apache.org/confluence/display/SENTRY/Sentry+Simple+Shell 
<https://cwiki.apache.org/confluence/display/SENTRY/Sentry+Simple+Shell>. While 
I was looking at the implementation of the SentryShell there is very little 
code that I actually used for the interactive shell, so there isn’t much 
intersection. Of course, if we decide to productize the interactive shell it 
makes a lot of sense to unify the two since they are doing, essentially, the 
same thing. But for now their overall organization is completely different.

> Is there a compelling reason to support both? Probably a good
> to get other folks input from the community.

Not really. But I agree that it would be great to hear from people who actually 
use it. That said, I do see an independent value in stand-alone GO-based CLI 
that works directly over Thrift.

> 
> Thanks,
> Lenni

Thanks for your input!

- Alex.

> 
> On Wed, Dec 14, 2016 at 5:19 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
> 
>> Inspired by SentryShell I wrote a prototype of interactive Sentry shell
>> where you have an open session and can issue CRUD commands for
>> roles/groups/privileges. I there any interest in making this integrated
>> into Sentry code base?
>> 

Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed

[Alexander Kolbasov]

> On Dec. 16, 2016, 7:05 p.m., Vadim Spector wrote:
> > It is probably ok, but ...
> > a) Did you investigate why service stayed alive? Perhaps, some knowledge 
> > about the implementation and its defficiencies to be gained there. Some 
> > unaccounted runaway threads? Or are we unable to termonaie some threads?
> > b) System.exit() means that Sentry service better be the only thing running 
> > in JVM. Which I presume is the case now. Are we ok with making that a 
> > requirement from now on?

For a) please see SENTRY-1526. The service stayed alive because of the deadlock.
b) I don't understand the question - what else can run in this JVM? This is a 
Sentry Server that we are trying to kill.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/#review159478
-------


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 54338: SENTRY-1515: Cleanup exception handling in SentryStore

[Alexander Kolbasov]

> On Dec. 5, 2016, 8:24 p.m., Vamsee Yarlagadda wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java,
> >  lines 188-194
> > <https://reviews.apache.org/r/54338/diff/1/?file=1575128#file1575128line188>
> >
> > 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?

Seems like there isn't much we can do anyway. We can exit but it wouldn't help 
either. ANyway, it is a different issue.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158054
-------


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
> 
>

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

[Alexander Kolbasov]

> On Dec. 5, 2016, 9:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java,
> >  line 79
> > <https://reviews.apache.org/r/54338/diff/1/?file=1575128#file1575128line79>
> >
> > do we still need ctors to be public, if we have public static create()?

It is used by UpdateForwarderWithHA, so it can't be private, but it can be 
package-private.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158055
---


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
> 
>

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54729/#review161110
---


Fix it, then Ship it!





sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
 (line 82)
<https://reviews.apache.org/r/54729/#comment232348>

iWhat is the point of multiplying 31 by one? Can you add some comment 
explaining the hash function?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java
 (line 64)
<https://reviews.apache.org/r/54729/#comment232349>

Can we pre-create both serializers as static final fields? This way we 
don't need to create them for every serde op. If we do that, we can just expose 
toString/fromString methodfs here.



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 (line 45)
<https://reviews.apache.org/r/54729/#comment232350>

Please change this to proper Javadoc and add description for parameters and 
return values.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
 (line 26)
<https://reviews.apache.org/r/54729/#comment232351>

Nit: need a space between  and Hive, otherwise it doesn't show as '<'



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
 (line 28)
<https://reviews.apache.org/r/54729/#comment232353>

It would be nice to show some example of permission change JSON object.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
 (line 29)
<https://reviews.apache.org/r/54729/#comment232352>

Nit: add 


- Alexander Kolbasov


On Jan. 10, 2017, 1:57 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54729/
> ---
> 
> (Updated Jan. 10, 2017, 1:57 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/ThriftSerializer.java
>  b66f70ba3f9cec6b44dc853ed09494be37303e2f 
>   
> 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 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

---
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.


Changes
---

Added documentation for solr tests changes


Bugs: SENTRY-1532
https://issues.apache.org/jira/browse/SENTRY-1532


Repository: sentry


Description
---

SENTRY-1532: Sentry Web UI isn't working


Diffs (updated)
-

  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 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

> On Jan. 9, 2017, 6:22 p.m., Mat Crocker wrote:
> > was the Web UI actually started and smoke tested, or just a unit test fix ?

I actually started it and accessed all entry points. Without the fix you can't 
use it.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55094/#review160918
---


On Jan. 5, 2017, 12:06 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55094/
> ---
> 
> (Updated Jan. 5, 2017, 12:06 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 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55246/#review161477
---



I would consider changing your interface a bit - instead of passing the update 
map or specific update to SentryStore, pass a TransactionBlock that should be 
executed as part of a SentrySTore transaction. This is a more generic approach. 
What do you think?

- Alexander Kolbasov


On Jan. 10, 2017, 2:31 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> ---
> 
> (Updated Jan. 10, 2017, 2:31 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra 
> transactions for a single permission update
> 
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
> 
> To persist single permission change, it needs to combine multiple things in a 
> single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
> 
> All the above steps are put into in a single transaction to guarantee that 
> notificationID handling is atomic.
> 
> 
> 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-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  24729282bf17960152f87b1d3124caeafb47e6b2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  3695709e03e683afe6196def53883e37e4910a1c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
>  2ff715f66ea6c2589a281b988438526546af3d3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  e678b575f86cd4797ad01f12e4a60fbeec9f84f5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  8789a48c47e439bc2791cfe5a3b716b586025a7a 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  caa3c58b6d2e5874bea52379b9dd549a76698b9b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a35c8d7dde485cf46d61968a211d1dbb6d9d6076 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
>  1c3a4f29984379f5246da8d85fe661320c8a1043 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 
> 
> Diff: https://reviews.apache.org/r/55246/diff/
> 
> 
> Testing
> ---
> 
> New unit tests added in TestHMSFollower.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

> On Jan. 12, 2017, 11:46 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/pom.xml, line 117
> > <https://reviews.apache.org/r/55094/diff/4/?file=1602136#file1602136line117>
> >
> > I understand that it was working before this changes.It should be 
> > working some time back right.
> > 
> > How was it working before with out these jetty-http and hetty-util 
> > dependencies.
> > 
> > Could that we because of using latest jetty.

No one knows when this stopped working. Part of the reason may be that upstream 
Sentry isn't actually used in any deployment, so it may have been broken for a 
long time.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55094/#review161459
-------


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: Review Request 53038: SENTRY-1507

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53038/
---

(Updated Jan. 11, 2017, 7:37 p.m.)


Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, Li Li, and 
Sravya Tirukkovalur.


Bugs: SENTRY-1507
https://issues.apache.org/jira/browse/SENTRY-1507


Repository: sentry


Description
---

SENTRY-1507 Sentry should use Datanucleus version of javax.jdo


Diffs (updated)
-

  sentry-service/sentry-service-server/pom.xml 
97bd326b0305303194a965a60b6afd0feefbbe0f 

Diff: https://reviews.apache.org/r/53038/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 53872: SENTRY-1525: Provide script to run Sentry directly from the repo

[Alexander Kolbasov]

---
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.


Changes
---

Addressed code review comments. Also made main class configurable via 
SENTRY_MAIN environment variable


Repository: sentry


Description
---

SENTRY-1525: Provide script to run Sentry directly from the repo


Diffs (updated)
-

  bin/run_sentry.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/53872/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Show groups for a given role

[Alexander Kolbasov]

Hi Jan,

You may check my new sentry command-line tool at 
https://github.com/akolb1/sentrytool . 
You can easily do it with this tool:

sentrytool group list -v 

Caveat - it doesn’t work in Kerberos setup.

I am not sure whether there is a more “official” way of doing that. There is a 
Sentry Shell which might be able to do something similar.

Best,

- Alex

> On Dec 3, 2016, at 3:16 AM, Jan Hentschel  
> wrote:
> 
> Hi,
> 
> 
> 
> I’m looking for a way to look up the access groups for a given role. I know 
> that I can list all the roles for a given group via
> 
> 
> 
> SHOW ROLE GRANT GROUP ;
> 
> 
> 
> What I want to do is something vice versa without directly querying the 
> Sentry database. Is there a command or JIRA ticket for this?
> 
> 
> 
> Best, Jan 
> 
> 
> 

Review Request 54409: SENTRY-1476: SentryStore is subject to JDQL injection

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54409/
---

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 53905: SENTRY-1518: Add metrics for SentryStore transactions

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53905/
---

(Updated Dec. 1, 2016, 3:08 a.m.)


Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan kumar 
kalvagadda, Sravya Tirukkovalur, Vamsee Yarlagadda, and Vadim Spector.


Repository: sentry


Description
---

SENTRY-1518: Add metrics for SentryStore transactions


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
 3d0a6ab0b514cb9fbead4b8e527f7cb84865ce28 

Diff: https://reviews.apache.org/r/53905/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 53520: SENTRY-1517: SentryStore shoud actually use function getMSentryRole to get roles

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53520/
---

(Updated Dec. 1, 2016, 2:56 a.m.)


Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan kumar 
kalvagadda, Li Li, Sravya Tirukkovalur, Vamsee Yarlagadda, and Vadim Spector.


Repository: sentry


Description
---

SENTRY-1517: SentryStore shoud actually use function getMSentryRole to get roles


Diffs (updated)
-

  
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/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/TestDelegateSentryStore.java
 ef32ad40783b3c23c262f73cfa449f2e15368852 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 66f05841b7fb009278c50d3e6c90e79063623c65 

Diff: https://reviews.apache.org/r/53520/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
---

(Updated Jan. 5, 2017, 8:28 p.m.)


Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
Yarlagadda, and Vadim Spector.


Bugs: SENTRY-1515
https://issues.apache.org/jira/browse/SENTRY-1515


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
 e8d0e2505c5b178cae3d27e7d85caa652f630a2d 

Diff: https://reviews.apache.org/r/54338/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 53920: SENTRY-1534: Oracle supports serializable instead of repeatable-read

[Alexander Kolbasov]

> On Nov. 30, 2016, 11:55 p.m., Hao Hao wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 167
> > <https://reviews.apache.org/r/53920/diff/2/?file=1571678#file1571678line167>
> >
> > Does "serializable" in Oracle mean the same thing as "repeatable" in 
> > other databases?

It isn't exactly the same. Oracle provides just two isolation levels - 
read-committed and serializable. Serializable is stronger than repeatable-read 
and will also protect us from updating the same row from two transactions at 
the same time.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53920/#review157501
---


On Jan. 5, 2017, 9:04 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53920/
> ---
> 
> (Updated Jan. 5, 2017, 9:04 a.m.)
> 
> 
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1534
> https://issues.apache.org/jira/browse/SENTRY-1534
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1534: Oracle supports serializable instead of repeatable-read
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  919fdaf11c36b412211176ede3c98170a2e34235 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f83d72160e1c2d3d1ec1b870ea4b4d5dda1a 
> 
> Diff: https://reviews.apache.org/r/53920/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 54729: SENTRY-1538: Create schema for storing HMS path change and Sentry permission change.

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54729/#review159412
---




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 (line 161)
<https://reviews.apache.org/r/54729/#comment231969>

Okease add javadocs for the new functions



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 (line 162)
<https://reviews.apache.org/r/54729/#comment231942>

Can we define "/" as a constant (if it isn't in some hadoop libraries that 
we use already)?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 (line 113)
<https://reviews.apache.org/r/54729/#comment231977>

Can we have a common static functions for serialization/deserialization 
that are used both here and in PathsUpdate so that we don't duplicate code?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 (line 44)
<https://reviews.apache.org/r/54729/#comment231973>

Please add javadoc for methods



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
 (line 45)
<https://reviews.apache.org/r/54729/#comment231975>

Naming - may be change to JSONSerialize and JSONDeserialize?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
 (line 26)
<https://reviews.apache.org/r/54729/#comment230427>

You need to use some HTML formatting here - use  and  for <> and  
for empty line - otherwise javadoc looks as one big long line.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
 (line 82)
<https://reviews.apache.org/r/54729/#comment230428>

What is the idea here of multiplying 31 by one?
And why changeId is not participating in the calculation?

Can we just use changeId as the hashcode?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
 (line 29)
<https://reviews.apache.org/r/54729/#comment230430>

Add 


- Alexander Kolbasov


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 52795: SENTRY-1499: Add feature flag for using NotificationLog

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52795/#review160763
---


Fix it, then Ship it!





sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 (line 166)
<https://reviews.apache.org/r/52795/#comment231959>

Nit: split long line



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 (line 165)
<https://reviews.apache.org/r/52795/#comment231957>

Seems like other constants are prefixed with 'sentry' - it may be better to 
follow the convention.


- Alexander Kolbasov


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 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160654
---



The change seems to check for other privilege correctness in addition to UNSET 
grant option - are you adding a fix for another problem here as well?


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 285)
<https://reviews.apache.org/r/54454/#comment231813>

It would be helpful to report what exactly is missing



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 286)
<https://reviews.apache.org/r/54454/#comment231814>

You don't need else clause here since the throw above will terminate, so 
you can just write this as two ifs.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 290)
<https://reviews.apache.org/r/54454/#comment231815>

Do we need to do anything here if the privilege set is empty?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1045)
<https://reviews.apache.org/r/54454/#comment231805>

Is there a test that uses it? Why is it visible for testing?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1046)
<https://reviews.apache.org/r/54454/#comment231804>

Please add javadoc for this function



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1047)
<https://reviews.apache.org/r/54454/#comment231807>

Can privileges be null here?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1050)
<https://reviews.apache.org/r/54454/#comment231803>

Why not just "return false"?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1057)
<https://reviews.apache.org/r/54454/#comment231800>

Use of overloading here is confusing - can you choose different names for 
these functions?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1058)
<https://reviews.apache.org/r/54454/#comment231801>

A simple

return privilege.getGrantOption() != TSentryGrantOption.UNSET

works just fine. And given that it is now a one-liner - do we really need a 
function for it? This will remove the problem with overloaded names as well.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1059)
<https://reviews.apache.org/r/54454/#comment231808>

Can privilege be null here?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1064)
<https://reviews.apache.org/r/54454/#comment231806>

Same comments as for validateGrantOption above.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1079)
<https://reviews.apache.org/r/54454/#comment231809>

Can it really be null?

This can be inlined directly in checkforMandatoryPrivilegeFeilds() above as

privilege.getServerName() != null &&   
privilege.getServerName().trim().length() > 0 &&
  privilege.getAction() != null &&
  privilege.getAction().trim().length() > 0)



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 (line 1083)
<https://reviews.apache.org/r/54454/#comment231811>

Do we allow any action as long as it is non-empty? Other code makes certain 
assumption about valid set of actions.


- Alexander Kolbasov


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

Re: Review Request 55190: SENTRY-1583: Refactor ZK/Curator code

[Alexander Kolbasov]

> On Jan. 7, 2017, 4:25 a.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java,
> >  line 251
> > <https://reviews.apache.org/r/55190/diff/3/?file=1598341#file1598341line251>
> >
> > 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?

We tell Curator framework to drop the leader status by returning from this 
callback.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55190/#review160806
-------


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 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55094/
---

(Updated Jan. 5, 2017, 12:06 a.m.)


Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, kalyan kumar 
kalvagadda, Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Fixed Jetty version for solr test


Bugs: SENTRY-1532
https://issues.apache.org/jira/browse/SENTRY-1532


Repository: sentry


Description
---

SENTRY-1532: Sentry Web UI isn't working


Diffs (updated)
-

  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 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

> On Jan. 4, 2017, 12:37 a.m., Vamsee Yarlagadda wrote:
> > sentry-tests/sentry-tests-solr/pom.xml, line 58
> > <https://reviews.apache.org/r/55094/diff/2/?file=1596481#file1596481line58>
> >
> > 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?

This is intentional. Tests run everything in a single JVM so later Jetty 
version conflicts wioth the Jetty version used by Solr. To work around this I 
added explicit older Jetty version in Solr sentry tests pom file.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55094/#review160457
-------


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 54409: SENTRY-1476: SentryStore is subject to JDQL injection

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54409/
---

(Updated Jan. 5, 2017, 1:22 a.m.)


Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Sravya 
Tirukkovalur, Vamsee Yarlagadda, and Vadim Spector.


Bugs: SENTRY-1476
https://issues.apache.org/jira/browse/SENTRY-1476


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

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review160548
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 2238)
<https://reviews.apache.org/r/54338/#comment231696>

How would you suggest to handle these errors inside these functions?

This changeset is restoring the status quo as it existed before SENTRY-1422 
and just makes exceptions more explicit.


- Alexander Kolbasov


On Dec. 14, 2016, 11:57 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> ---
> 
> (Updated Dec. 14, 2016, 11:57 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
>  e8d0e2505c5b178cae3d27e7d85caa652f630a2d 
> 
> Diff: https://reviews.apache.org/r/54338/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 55092: SENTRY-1581: Provide Log4J metrics reporter

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55092/
---

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

Review Request 55094: SENTRY-1532: Sentry Web UI isn't working

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55094/
---

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
-

  pom.xml b27197d957bfc72933fe00c42ff24af8971b0bed 
  sentry-service/sentry-service-server/pom.xml 
97bd326b0305303194a965a60b6afd0feefbbe0f 

Diff: https://reviews.apache.org/r/55094/diff/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review160430
---


Ship it!




Ship It!

- Alexander Kolbasov


On Jan. 3, 2017, 8:15 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, 8:15 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 55190: SENTRY-1583: Refactor ZK/Curator code

[Alexander Kolbasov]

> On Jan. 6, 2017, 5:20 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java,
> >  line 173
> > <https://reviews.apache.org/r/55190/diff/2/?file=1598234#file1598234line173>
> >
> > I wonder why this method is public and the previous one is not - is 
> > that intentional?

ANother one isn't used by anyone yet - this may change later.


> On Jan. 6, 2017, 5:20 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java,
> >  line 122
> > <https://reviews.apache.org/r/55190/diff/2/?file=1598240#file1598240line122>
> >
> > I've checked how this Lock is used, and it looks like there is nothing 
> > sophisticated about it. It can be replaced with simple synchronized {} 
> > (maybe still on a separate 'Object lock'), which will make the code easier 
> > to read and less error-prone.

There is nothing sofisticated but we need condition variable associated with 
the lock to send signal - see deactivate() and takeLeadership() which calls 
cond.await() - that's the reason to have a lock.


> On Jan. 6, 2017, 5:20 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java,
> >  line 171
> > <https://reviews.apache.org/r/55190/diff/2/?file=1598240#file1598240line171>
> >
> > A more sensible name for this method would be 'finishInitialization()'. 
> > I am not really convinced that this functionality should be in a separate 
> > method - the javadoc doesn't give any profound reasons. But, well, up to 
> > you.

The reason is not to publish 'this' for another thread until it isn't fully 
constrcted. This may cause visibility problems.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55190/#review160672
---


On Jan. 6, 2017, 2:14 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, 2:14 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/

Re: Review Request 55190: SENTRY-1583: Refactor ZK/Curator code

[Alexander Kolbasov]

---
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.


Changes
---

Addressed code review comments from Misha


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 (updated)
-

  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 55190: SENTRY-1583: Refactor ZK/Curator code

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55190/
---

(Updated Jan. 6, 2017, 2:14 a.m.)


Review request for sentry, Colin Ma, Misha Dmitriev, Dapeng Sun, Hao Hao, 
kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comments from Misha


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 (updated)
-

  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 52795: SENTRY-1499: Add feature flag for using NotificationLog

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52795/#review159534
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 52)
<https://reviews.apache.org/r/52795/#comment230546>

Rather than removing all class comments can you update them to reflect the 
new semantics?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 59)
<https://reviews.apache.org/r/52795/#comment230547>

It seems that this can be private and final - I don't see any usages 
outside the class.



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 72)
<https://reviews.apache.org/r/52795/#comment230550>

Consider using static import for CLASS_SPLITTER



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 80)
<https://reviews.apache.org/r/52795/#comment230552>

it may be better to use \" for the class name instead of square brackets.



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 90)
<https://reviews.apache.org/r/52795/#comment230553>

please tell what plugin failed to initialize.



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 99)
<https://reviews.apache.org/r/52795/#comment230559>

This code is repeated everywhere - is it possible to handle it in a generic 
way?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 110)
<https://reviews.apache.org/r/52795/#comment230561>

If some plugin fails, is there a need to undo any work?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 (line 114)
<https://reviews.apache.org/r/52795/#comment230560>

What is ment by drop here? The method is about creating a table.


- Alexander Kolbasov


On Dec. 16, 2016, 10:03 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52795/
> ---
> 
> (Updated Dec. 16, 2016, 10:03 p.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.
> 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 54819: SENTRY-1526: Sentry processed stays alive after being killed (Fixing Mismerge) (Alexander Kolbasov via Vamsee Yarlagadda)

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54819/#review159491
---


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 16, 2016, 8:41 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54819/
> ---
> 
> (Updated Dec. 16, 2016, 8:41 p.m.)
> 
> 
> 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 57232: SENTRY-1613: Add propagating logic for Perm/Path updates in Sentry service

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57232/#review169982
---


Ship it!




Ship It!

- Alexander Kolbasov


On March 24, 2017, 1:12 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57232/
> ---
> 
> (Updated March 24, 2017, 1:12 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the logic for propagating Sentry Permissions or Sentry representation of 
> HMS Paths, from a persistent storage instead of in memoery cache of Sentry 
> service, to a Sentry consumer such as HDFS NameNode. It includes:
> 1. Delta update retrieval logic from persistent storage.
> 2. Propagation logic for consumer to get the delta update or a complete 
> snapshot.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java
>  0e40756b0da0214e83b7127dcd8816f30c9692c8 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java
>  69aa098bc71362b1aba0ad219483cfce7d389964 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java
>  16ffa1b530a47b425333ada047d2a7ec96d260ef 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  16a1604700079cb37f6fbaeceae92c67b2c08d8b 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  3017c9edb2217cd0152183f927691311a276e850 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  e4f3f580ee0a6bf83d5a32001bff130c0bff13aa 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  be145695af35b34f409e1f8b6e49da247f78e7a0 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  029f9d5dc89493f7e7086b80387f71fef1ac0805 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  22c57690a0f74822187335f97cf1eede8d415acf 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java
>  03c67d674813c59a23c5bc6c57671d79dfd235a0 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
>  d12b1342116217685a46367567e2c0f8d7c288e5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  6ea6d3f44d7a2c7cd2b375af74f3b19d5731aed6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  f3f51dabdd610fbf6095a9d0494de78ee8f3ded7 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  75f855caec1060f9158562aeb81ec53d8a679941 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  5e8b2fac76d6dcf7aee34729e86755797af64406 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  1b5eb537ec780e4b2444a7b6744ed4aa75da2d08 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  859c8f882db76e46297c1f6377de32bc2b08f485 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  c7912726cda157761753b71d43cfb272ecc5e8c4 
> 
> 
> Diff: https://reviews.apache.org/r/57232/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 57570: SENTRY-1644. Partition ACLs disappear after renaming Hive table with partitions

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57570/#review170064
---




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 181 (patched)
<https://reviews.apache.org/r/57570/#comment242838>

There is no documentation in this class about the parent-child 
relationships - since you just looked at this - can you add some text either 
here or at the class level explaining what is a parent and what is a child?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 182 (patched)
<https://reviews.apache.org/r/57570/#comment242839>

What does it mean for path elements to point to the direct parent? May be 
you can put some example here? Also, from the code it looks like the elements 
are ordered in a specific way - can you document this as well and/or show 
example?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 184 (patched)
<https://reviews.apache.org/r/57570/#comment242842>

From the code it returns the parent of the *last* element



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 195 (original), 214 (patched)
<https://reviews.apache.org/r/57570/#comment242845>

Why do you need subList() here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 325 (patched)
<https://reviews.apache.org/r/57570/#comment242848>

Didn't you just check for this in line 319?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 298 (original), 336 (patched)
<https://reviews.apache.org/r/57570/#comment242847>

This isn't related, but since you are touching this file - why do you call 
remove() if you know that getChildren() is empty? Can it be that the condition 
is reversed here by any chance?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 579 (original), 624 (patched)
<https://reviews.apache.org/r/57570/#comment242851>

formatting - space between // and text



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 587 (original), 629 (patched)
<https://reviews.apache.org/r/57570/#comment242853>

    Why do you need subList() here?


- Alexander Kolbasov


On March 22, 2017, 9:27 p.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57570/
> ---
> 
> (Updated March 22, 2017, 9:27 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Hao Hao.
> 
> 
> Bugs: SENTRY-1644
> https://issues.apache.org/jira/browse/SENTRY-1644
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> After rename Hive table with partitions, ACLs on the partition paths 
> disappear.
> 
> It is caused by the fact that renaming on sentry does not move any 
> sub-namespace.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6d2ab23c3f65af5430ea02563e13c4c4c49aa1c6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  0dee1028ac852b12b109e875b2cbbb0bd2efbe03 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  d079628a0e549e3b179c7a321f82bcf6b580da43 
> 
> 
> Diff: https://reviews.apache.org/r/57570/diff/2/
> 
> 
> Testing
> ---
> 
> mvn test -Dtest=TestHMSPaths
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Review Request 58087: SENTRY-1676 FullUpdateInitializer#createInitialUpdate should not throw RuntimeException

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58087/
---

Review request for sentry, Hao Hao, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-1676
https://issues.apache.org/jira/browse/SENTRY-1676


Repository: sentry


Description
---

SENTRY-1676 FullUpdateInitializer#createInitialUpdate should not throw 
RuntimeException


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 146cea2b9467ce82b69bbf402933b1aa350bcd46 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 6c14f5e46aad4223347d8d057188d31efbb68ed8 


Diff: https://reviews.apache.org/r/58087/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58087: SENTRY-1676 FullUpdateInitializer#createInitialUpdate should not throw RuntimeException

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58087/
---

(Updated March 30, 2017, 6:55 p.m.)


Review request for sentry, Hao Hao, kalyan kumar kalvagadda, and Na Li.


Changes
---

Log exception as error


Bugs: SENTRY-1676
https://issues.apache.org/jira/browse/SENTRY-1676


Repository: sentry


Description
---

SENTRY-1676 FullUpdateInitializer#createInitialUpdate should not throw 
RuntimeException


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 146cea2b9467ce82b69bbf402933b1aa350bcd46 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 6c14f5e46aad4223347d8d057188d31efbb68ed8 


Diff: https://reviews.apache.org/r/58087/diff/2/

Changes: https://reviews.apache.org/r/58087/diff/1-2/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57901/#review170070
---



I think that this should be split into two parts:

1) Move files around without changing them (except as needed for the mvoe)
2) Actually changing files

The first one should be very straightforward and the second one will 
concentrate on actual HA work. This would be much easier to review.


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
Lines 21 (patched)
<https://reviews.apache.org/r/57901/#comment242858>

Please add javadoc for the class



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
Lines 28 (patched)
<https://reviews.apache.org/r/57901/#comment242857>

This constructor is never used



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
Lines 22 (patched)
<https://reviews.apache.org/r/57901/#comment243520>

Please do not comment out imports, remove them instead.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
Lines 62 (patched)
<https://reviews.apache.org/r/57901/#comment243521>

Our convention is to use clientObject rather then client_object


- Alexander Kolbasov


On March 29, 2017, 11:32 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated March 29, 2017, 11:32 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, 
> and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> 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.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 9d18063 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  6cea596 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  636de40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  12175f7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  038bca7 
>   
> 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/SentryServiceClientTransportDefaultImpl.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.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
>  28b1224 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  2a18b15 
>   
> sentry-hdfs/sentry-h

[DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

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

Re: Review Request 57232: SENTRY-1613: Add propagating logic for Perm/Path updates in Sentry service

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57232/#review169817
---




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 36 (patched)
<https://reviews.apache.org/r/57232/#comment242455>

This class and its methods can be package-private



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 49 (patched)
<https://reviews.apache.org/r/57232/#comment242460>

I had this comment before - it is cleaner to be explicit whether we return 
a list of deltas or a full update - either by using different functions or 
returning a special result.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 67 (patched)
<https://reviews.apache.org/r/57232/#comment242456>

Use collections.emptyList() instead



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 72 (patched)
<https://reviews.apache.org/r/57232/#comment242458>

Add extra parenthesis for clarity: 

if ((seqNum != SentryStore.INIT_CHANGE_ID) &&
deltaRetriever.isDeltaAvailable(seqNum)) {



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
Lines 42 (patched)
<https://reviews.apache.org/r/57232/#comment242473>

Would it make sense to add a metric to measure time to get it? If you don't 
want to add it now, please don't forget to file a follow-up JIRA.

Same for PermDeltaRetriever.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
Lines 43 (patched)
<https://reviews.apache.org/r/57232/#comment242467>

You can return Collections.emptyList() if mSentryPathChange is empty.
Otherwise it is better to specify exact size for the update:

Collection updates = new 
ArrayList<>(mSentryPathChanges.size());

Same comment for PermDeltaRetriever.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 75 (patched)
<https://reviews.apache.org/r/57232/#comment242475>

Do we need to construct a new array each time? Can we have this as a static 
constant? At the minimum, use static string for "/".



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 77 (patched)
<https://reviews.apache.org/r/57232/#comment242476>

Since there is no concurrency here, why should we create a new lock each 
time? A single lock would work as well.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Line 79 (original), 79 (patched)
<https://reviews.apache.org/r/57232/#comment242477>

can we use Collections.emptyList() instead?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Line 49 (original), 49 (patched)
<https://reviews.apache.org/r/57232/#comment242478>

Are we still using SentryPlugin?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3296 (patched)
<https://reviews.apache.org/r/57232/#comment242481>

Why use 't' as the parameter rather then changeId?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3299 (patched)
<https://reviews.apache.org/r/57232/#comment242482>

simply return !changes.isEMpty()



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3380 (patched)
<https://reviews.apache.org/r/57232/#comment242484>

instead of t use a better name  like changeId



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3403 (patched)
<https://reviews.apache.org/r/57232/#comment242486>

Add parenthesis:

if (pathChanges.size() < ((curChangeID - changeID) + 1)) {



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3406 (patched)
<https://reviews.apache.org/r/57232/#comment242487>

return Collections.emptyList()



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3432 (patched)
<https://reviews.apache.org/r/57232/#comment242488>

Add parenthesis



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3435 (patched)
<https://reviews.apache.org/r/57232/#comment242489>

return Collections.emptyList()


- Alexander Kolbasov


On March 22, 2017, 11:08 p.m., Hao 

Review Request 58069: SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58069/
---

Review request for sentry.


Bugs: SENTRY-1670
https://issues.apache.org/jira/browse/SENTRY-1670


Repository: sentry


Description
---

SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 bbfa713262c5b4d5cecccd95c823a78c9149752c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 6ed2c781b4ac8819f6d17a249c33e32f0116e15a 


Diff: https://reviews.apache.org/r/58069/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review169846
---


Ship it!




The changes are good, but do you need to address SENTRY-1675?

- Alexander Kolbasov


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> ---
> 
> (Updated March 22, 2017, 11:05 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1639
> https://issues.apache.org/jira/browse/SENTRY-1639
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1639 Refactored the usage of configuration constants related to 
> transport These are subset of changes done for SENTRY-1592.
> 
> 
> Diffs
> -
> 
>   
> 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/utils/SentryConstants.java
>  4ed1361 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java
>  085971b 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  03bf39e 
>   
> 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/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  2cf748e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 57657: SENTRY-1658: Fixed possible null pointer dereference in SentryShellHive

[Alexander Kolbasov]

> On March 15, 2017, 7:07 p.m., Alexander Kolbasov wrote:
> > Please fix the commit message
> 
> Jan Hentschel wrote:
> What is wrong with the commit message?

The commit message should be 
SENTRY-1658: Null pointer derefeence in SentryShellHive


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57657/#review169047
---


On March 15, 2017, 6:41 p.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57657/
> ---
> 
> (Updated March 15, 2017, 6:41 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1658
> https://issues.apache.org/jira/browse/SENTRY-1658
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Fixed a possible null pointer dereference in **SentryShellHive**.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-client/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java
>  dc7f829 
> 
> 
> Diff: https://reviews.apache.org/r/57657/diff/1/
> 
> 
> Testing
> ---
> 
> Successful run of **mvn clean install**.
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Re: Review Request 57375: Unable to truncate table .; from "default" databases

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57375/#review169084
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 253 (patched)
<https://reviews.apache.org/r/57375/#comment241401>

You use ast.getChild(0)getCHild(0) several times - would it make sense to 
assign it to a variable with a meaningful name?


- Alexander Kolbasov


On March 7, 2017, 1:41 p.m., Yongzhi Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57375/
> ---
> 
> (Updated March 7, 2017, 1:41 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Please review fix for SENTRY-1646. Fix the issue by getting the currOutDB 
> inform from qualified table name if exists.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  d269d5fe0bfd3e8638fcec15d426862f4f15ae35 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtTableScopePart2.java
>  8eb0bd69158805a3fbcb6c88d61b26ef922997ef 
> 
> 
> Diff: https://reviews.apache.org/r/57375/diff/1/
> 
> 
> Testing
> ---
> 
> Add the test case in Unit test.
> 
> 
> Thanks,
> 
> Yongzhi Chen
> 
>

Re: Review Request 57657: SENTRY-1658: Fixed possible null pointer dereference in SentryShellHive

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57657/#review169046
---


Ship it!




Ship It!

- Alexander Kolbasov


On March 15, 2017, 6:41 p.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57657/
> ---
> 
> (Updated March 15, 2017, 6:41 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1658
> https://issues.apache.org/jira/browse/SENTRY-1658
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Fixed a possible null pointer dereference in **SentryShellHive**.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-client/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java
>  dc7f829 
> 
> 
> Diff: https://reviews.apache.org/r/57657/diff/1/
> 
> 
> Testing
> ---
> 
> Successful run of **mvn clean install**.
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Re: Review Request 57654: SENTRY-1663: Moved mutable field in UpdateableAuthzPermissions to immutable

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57654/#review169048
---


Ship it!




Please fix the commit message

- Alexander Kolbasov


On March 15, 2017, 5:33 p.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57654/
> ---
> 
> (Updated March 15, 2017, 5:33 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1663
> https://issues.apache.org/jira/browse/SENTRY-1663
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Moved the mutable field **ACTION_MAPPING** in **UpdateableAuthzPermissions** 
> to a immutable field.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  3d3fc8d 
> 
> 
> Diff: https://reviews.apache.org/r/57654/diff/1/
> 
> 
> Testing
> ---
> 
> Successful run of **mvn clean install**.
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Re: Review Request 58069: SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58069/
---

(Updated April 4, 2017, 8:33 p.m.)


Review request for sentry, Hao Hao and kalyan kumar kalvagadda.


Changes
---

Returned ID types from long to Long


Bugs: SENTRY-1670
https://issues.apache.org/jira/browse/SENTRY-1670


Repository: sentry


Description
---

SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 bbfa713262c5b4d5cecccd95c823a78c9149752c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 6ed2c781b4ac8819f6d17a249c33e32f0116e15a 


Diff: https://reviews.apache.org/r/58069/diff/3/

Changes: https://reviews.apache.org/r/58069/diff/2-3/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58069: SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58069/
---

(Updated April 4, 2017, 8:21 p.m.)


Review request for sentry, Hao Hao and kalyan kumar kalvagadda.


Changes
---

Addressed code review comments from Hao Hao:
  changed return type from Long to long for functions that get notification IDs


Bugs: SENTRY-1670
https://issues.apache.org/jira/browse/SENTRY-1670


Repository: sentry


Description
---

SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 bbfa713262c5b4d5cecccd95c823a78c9149752c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 6ed2c781b4ac8819f6d17a249c33e32f0116e15a 


Diff: https://reviews.apache.org/r/58069/diff/2/

Changes: https://reviews.apache.org/r/58069/diff/1-2/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 57570: SENTRY-1644. Partition ACLs disappear after renaming Hive table with partitions

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57570/#review171020
---


Fix it, then Ship it!




Ship It!


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 112 (patched)
<https://reviews.apache.org/r/57570/#comment243852>

Each individual entry



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 114 (patched)
<https://reviews.apache.org/r/57570/#comment243853>

Please axtend this comment to show how the tree is represented (a small 
example would help) - it isn't clear what is rthe relationship with Entry and 
the HDFS path. 

That aside - the full hdfs path can contain uri prefix - is it always 
trimmed somewhere before it reaches here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 339 (patched)
<https://reviews.apache.org/r/57570/#comment243851>

should it be private?
The return value is never used - is it intended?


- Alexander Kolbasov


On March 28, 2017, 12:24 a.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57570/
> ---
> 
> (Updated March 28, 2017, 12:24 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Hao Hao.
> 
> 
> Bugs: SENTRY-1644
> https://issues.apache.org/jira/browse/SENTRY-1644
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> After rename Hive table with partitions, ACLs on the partition paths 
> disappear.
> 
> It is caused by the fact that renaming on sentry does not move any 
> sub-namespace.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6d2ab23c3f65af5430ea02563e13c4c4c49aa1c6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  0dee1028ac852b12b109e875b2cbbb0bd2efbe03 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  d079628a0e549e3b179c7a321f82bcf6b580da43 
> 
> 
> Diff: https://reviews.apache.org/r/57570/diff/3/
> 
> 
> Testing
> ---
> 
> mvn test -Dtest=TestHMSPaths
> 
> 
> File Attachments
> 
> 
> SENTRY-1644.003-master.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/03/28/1b82d5bc-0267-42d3-9a9a-9515ed7cb25e__SENTRY-1644.003-master.patch
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Re: Review Request 58164: SENTRY-1638 Update SQL script of MSentryPathChange table to add a column for notification ID

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58164/#review171008
---



For tables that are new in sentry-ha-redesign branch, shouldn't we just update 
the initial creation scripts rather then add new scripts to fix the tables?

- Alexander Kolbasov


On April 4, 2017, 5:05 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58164/
> ---
> 
> (Updated April 4, 2017, 5:05 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.
> 
> 
> Bugs: SENTRY-1638
> https://issues.apache.org/jira/browse/SENTRY-1638
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> UpdateD SQL script of MSentryPathChange table to add a column for 
> notification ID. Wich include changes to jdo defination and also 
> MSentryPermChange class.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 547bbe8136186658e7fe76ab24934157ea5300ff 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  5376c166659b3e8e373c8c20818f5b1290af90c9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-1.7.0-to-1.8.0.sql
>  dc3021a57ac9ea8145379c437ae6a94131c9ae5e 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-1.7.0-to-1.8.0.sql
>  ef1c651da8998eb57b5d0ca3463317d07c1b3a24 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-1.7.0-to-1.8.0.sql
>  f4a50de14dd0f961befaded394c6a01c2f459317 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-1.7.0-to-1.8.0.sql
>  db010bcd7792c9370478a2061e22d14bb78e3184 
> 
> 
> Diff: https://reviews.apache.org/r/58164/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 57570: SENTRY-1644. Partition ACLs disappear after renaming Hive table with partitions

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57570/#review171052
---


Ship it!




Ship It!

- Alexander Kolbasov


On April 4, 2017, 8:54 p.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57570/
> ---
> 
> (Updated April 4, 2017, 8:54 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Hao Hao.
> 
> 
> Bugs: SENTRY-1644
> https://issues.apache.org/jira/browse/SENTRY-1644
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> After rename Hive table with partitions, ACLs on the partition paths 
> disappear.
> 
> It is caused by the fact that renaming on sentry does not move any 
> sub-namespace.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6d2ab23c3f65af5430ea02563e13c4c4c49aa1c6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  0dee1028ac852b12b109e875b2cbbb0bd2efbe03 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  d079628a0e549e3b179c7a321f82bcf6b580da43 
> 
> 
> Diff: https://reviews.apache.org/r/57570/diff/4/
> 
> 
> Testing
> ---
> 
> mvn test -Dtest=TestHMSPaths
> 
> 
> File Attachments
> 
> 
> SENTRY-1644.003-master.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/03/28/1b82d5bc-0267-42d3-9a9a-9515ed7cb25e__SENTRY-1644.003-master.patch
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Re: [DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

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 <ak...@cloudera.com> 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

Re: Review Request 58166: SENTRY-1692 ZK namespace configuration doesn't work

[Alexander Kolbasov]

> On April 4, 2017, 6:07 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 96 (original)
> > <https://reviews.apache.org/r/58166/diff/1/?file=1684224#file1684224line96>
> >
> > Not quite follow the change. If we are complete remove namespace here, 
> > why do we still keep the namespace config in HAContext? And why do we need 
> > it?

I update the JIRA to clarify this. Copying from there:
There are two problems:
1) The namespace used by HAContext shouldn't start with "/", so in the fix we 
remove the initial slash if it is present
2) The LeaderStatusMonitor shouldn't use prefix at all - it appends "/leader" 
to the namespace.
The patch addresses both issues.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58166/#review170957
---


On April 4, 2017, 5:03 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58166/
> ---
> 
> (Updated April 4, 2017, 5:03 a.m.)
> 
> 
> Review request for sentry, Hao Hao, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: SENTRY-1692
> https://issues.apache.org/jira/browse/SENTRY-1692
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1692 ZK namespace configuration doesn't work
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
>  00eec4eab066829ae1226d0ba3485eab7bd9eeb2 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  1d79bd1f722505e56941dfd131d782e518515dc4 
> 
> 
> Diff: https://reviews.apache.org/r/58166/diff/1/
> 
> 
> Testing
> ---
> 
> I tested this manually by running the server with namespace provided in the 
> config.
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58069: SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)

[Alexander Kolbasov]

> On April 4, 2017, 6:31 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3167 (original), 3187 (patched)
> > <https://reviews.apache.org/r/58069/diff/1/?file=1680728#file1680728line3187>
> >
> > Why do we change from 'Long' to 'long' here? it would result in extra 
> > unbox and autobox.

I did to get rid of IntelliJ warning about autoboxing of EMPTY_CHANGE_ID below. 
This isn't the usual case so we could ignore the warning, but when I checked 
usages it turns out that everyone is using getLastProcessedChangeIDCore() as 
long, so everyone unboxes anyway, so the net result is zero.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58069/#review170958
-------


On March 30, 2017, 5:25 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58069/
> ---
> 
> (Updated March 30, 2017, 5:25 a.m.)
> 
> 
> Review request for sentry, Hao Hao and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-1670
> https://issues.apache.org/jira/browse/SENTRY-1670
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1670 Expose current HMS notification ID as a Sentry gauge (metric)
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  bbfa713262c5b4d5cecccd95c823a78c9149752c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  6ed2c781b4ac8819f6d17a249c33e32f0116e15a 
> 
> 
> Diff: https://reviews.apache.org/r/58069/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58131: SENTRY-1680: Removed MetastoreCacheInitializer

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58131/#review170998
---


Ship it!




Ship It!

- Alexander Kolbasov


On April 4, 2017, 11:59 a.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58131/
> ---
> 
> (Updated April 4, 2017, 11:59 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1680
> https://issues.apache.org/jira/browse/SENTRY-1680
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Removed the **MetastoreCacheInitializer** class and its dependend code.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastoreCacheInitializer.java
>  f9664f0 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestMetastoreCacheInitializer.java
>  fbaaa2c 
> 
> 
> Diff: https://reviews.apache.org/r/58131/diff/2/
> 
> 
> Testing
> ---
> 
> Successful run of **mvn clean install**.
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of MSentryPermChange/MSentryPathChange may be error-prone

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58281/#review171494
---


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2592 (patched)
<https://reviews.apache.org/r/58281/#comment244423>

I'd suggest to use barrier to make sure all threads start racing at the 
same time. This will improve chance of getting conflicts.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2606 (patched)
<https://reviews.apache.org/r/58281/#comment244424>

shutdown will cancel all pending jobs - do you need to wait until all are 
done? Otherwise you might miss some updates



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2616 (patched)
<https://reviews.apache.org/r/58281/#comment244425>

it may be better to log it rather then print to stdout


- Alexander Kolbasov


On April 10, 2017, 7:54 p.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> ---
> 
> (Updated April 10, 2017, 7:54 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When it relies on the SQL auto increment primary key as ChangeID, it can not 
> guarentee the consectivity of the IDs, because while each transaction claims 
> new ID from the table counter, the concurrent transactions which were not 
> success would not return the claimed changeIDs to the pool, thus it can not 
> guarateen the consectivity of change IDs.
> 
> This patch changes to use application logic to force the consectivity of IDs.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  a0d34459d7b2f70e863ef6e078401df81381c91b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  476fbcb2ad26de23757842111beb12b154e1562b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
>  f590a5296c047e1acedd39a4f2e4f1de98008d32 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  802b9c6cbf8e9ad015e37037b809b58c956de746 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58281/diff/2/
> 
> 
> Testing
> ---
> 
> Add a new test to conurrently insert changes. 
> 
> mvn test -Dtest=TestSentryStore.
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171256
---


Ship it!




Ship It!

- Alexander Kolbasov


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 7, 2017, 5: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
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58267/#review171509
---


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 69 (patched)
<https://reviews.apache.org/r/58267/#comment27>

Please provide javadoc for this new function
And, again, the name of this function seems wrong


- Alexander Kolbasov


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> ---
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
> https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Older JDO defination had issues handling multiple paths associtaed with one 
> Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is 
> associated with more than one path.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  56e456eb37df6d0a14402f57dbc400be9b72 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  802b9c6cbf8e9ad015e37037b809b58c956de746 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
>  1883626262bf4f4936f156a7ac74365b9b5873df 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql
>  7de9751892a8ff84067f67d542ac58d33e9148d8 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql
>  adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 547bbe8136186658e7fe76ab24934157ea5300ff 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  5376c166659b3e8e373c8c20818f5b1290af90c9 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> ---
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 58121: SENTRY-1677 Add metrics to measure how much time to get Delta Path/Perm Updates

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58121/
---

Review request for sentry, Hao Hao, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-1677
https://issues.apache.org/jira/browse/SENTRY-1677


Repository: sentry


Description
---

SENTRY-1677 Add metrics to measure how much time to get Delta Path/Perm Updates


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
 cea5b9de7e1415946ae2a3a76e53e50d5011bf7f 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
 9649b0280341fb7f6e1d745c23936f2d635477e0 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
 28bf20edb63b08a92b6ec060bc76934dea82f746 


Diff: https://reviews.apache.org/r/58121/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58094: SENTRY-1683 MetastoreCacheInitializer has a race condition in handling results list

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58094/
---

(Updated March 31, 2017, 10:36 p.m.)


Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, and Vadim 
Spector.


Changes
---

Removed extra serialized() which is no longer needed


Bugs: SENTRY-1683
https://issues.apache.org/jira/browse/SENTRY-1683


Repository: sentry


Description
---

SENTRY-1683 MetastoreCacheInitializer has a race condition in handling results 
list


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastoreCacheInitializer.java
 f9664f02d076f03a6481adbac24cefd72e90e152 


Diff: https://reviews.apache.org/r/58094/diff/2/

Changes: https://reviews.apache.org/r/58094/diff/1-2/


Testing
---


Thanks,

Alexander Kolbasov

Review Request 58093: SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw RuntimeExceptio

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58093/
---

Review request for sentry, Hao Hao, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-1676
https://issues.apache.org/jira/browse/SENTRY-1676


Repository: sentry


Description
---

SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw 
RuntimeExceptio


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 146cea2b9467ce82b69bbf402933b1aa350bcd46 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 6c14f5e46aad4223347d8d057188d31efbb68ed8 


Diff: https://reviews.apache.org/r/58093/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Review Request 58130: SENTRY-1684 FullUpdateInitializer has a race condition in handling results list

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58130/
---

Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, and Vadim 
Spector.


Bugs: SENTRY-1684
https://issues.apache.org/jira/browse/SENTRY-1684


Repository: sentry


Description
---

SENTRY-1684 FullUpdateInitializer has a race condition in handling results list


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 146cea2b9467ce82b69bbf402933b1aa350bcd46 


Diff: https://reviews.apache.org/r/58130/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171666
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 108 (patched)
<https://reviews.apache.org/r/58221/#comment244662>

No need to return at the end of the function.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 376 (original), 383 (patched)
<https://reviews.apache.org/r/58221/#comment244663>

As you are fixing this, can you fix this line as well - we know that dbNam 
== null so we shouldn't try to convert it to string.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 77 (patched)
<https://reviews.apache.org/r/58221/#comment244666>

Style - it is better to have field declarations first, followed by methods. 
The close() method is usually somewhere closer to the end but we don't have any 
specific policy on that.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 78 (patched)
<https://reviews.apache.org/r/58221/#comment244665>

Do you ever expect concurrent calls to close() or not?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 81 (patched)
<https://reviews.apache.org/r/58221/#comment244664>

Note that you need to stop the executor first. Otherwise while you are 
calling close() it may run HMSFOllower which isn't good.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 92 (patched)
<https://reviews.apache.org/r/58221/#comment244667>

Can you call awaitTermination() before you called shutdown()?

The doc says:

Blocks until all tasks have completed execution after a shutdown request, 
or the timeout occurs, or the current thread is interrupted, whichever happens 
first.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 96 (patched)
<https://reviews.apache.org/r/58221/#comment244668>

I think this should never happen.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 99 (patched)
<https://reviews.apache.org/r/58221/#comment244670>

Should we call shutdownNow() in this case?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 145 (patched)
<https://reviews.apache.org/r/58221/#comment244673>

Why do you need this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 160 (patched)
<https://reviews.apache.org/r/58221/#comment244674>

Why do you need this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 350 (patched)
<https://reviews.apache.org/r/58221/#comment244675>

This is most likely wrong. What does the cancel of the future do? What task 
does it actually cancel given that this is periodic executor? U think you 
should just use shutdown on the executor instead of this.


- Alexander Kolbasov


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 12, 2017, 3:47 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
>  6c14f5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> > Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
> It is possible. I will add "synchronized" in front of it.

Under what circumstances is it possible? If it is possible, it may not be 
sufficient to add synchronized() - you may need to protect access to the 
variables as well, but I a not convinced that it is possible. Do we run our 
tests in parallel?


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> > I think this should never happen.
> 
> Na Li wrote:
> From API, it could be thrown

from the API - yes, but this shouldn't happen in our case, so we should log 
something about the fact that this should never happen.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171666
---


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 12, 2017, 5:38 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/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> > This is most likely wrong. What does the cancel of the future do? What 
> > task does it actually cancel given that this is periodic executor? U think 
> > you should just use shutdown on the executor instead of this.

It seems to be not exactly wrong, but makes things more complicated then they 
should be. You need to shutdown the executor on exit anyway, so why cancel 
scheduling using futures?


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171666
---


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 12, 2017, 3:47 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
>  6c14f5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58093: SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw RuntimeExceptio

[Alexander Kolbasov]

> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 137 (original), 135 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681748#file1681748line141>
> >
> > do you want to print out the value of "retries +1"? Does "retries + 1" 
> > gives you want you want?

It does - otherwise I'll see that it failed after 0 retries


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 322 (original), 319 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681748#file1681748line327>
> >
> > do you need to update testing cases in TestFullUpdateInitializer that 
> > verifies the exception type?

Hmm, the tests are passing fine. I guess tests do not check for this.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 303 (original), 303 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line303>
> >
> > since you changed the possible exception, you need to update the 
> > commnet at line 303

Good point, this should be updated.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 310 (original), 310 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line310>
> >
> > It would be easier to debug if you log the beginning of getting full 
> > snapshot.

Sure, we can add the log.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 312 (original), 312 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line312>
> >
> > My impression is that it takes some time to get the full snapshot. 
> > Should we measure the time it takes to get the full snapshot? And show the 
> > duration in the log message?

I think we should provide this as a metric. Logs are time stamped - this could 
be sufficient, but having this as a metric is useful - then we can get it 
afterwords. I'll file a JIRA on this.


- Alexander


-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58093/#review171022
---


On March 31, 2017, 5:07 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58093/
> ---
> 
> (Updated March 31, 2017, 5:07 a.m.)
> 
> 
> Review request for sentry, Lei Xu, Hao Hao, kalyan kumar kalvagadda, and Na 
> Li.
> 
> 
> Bugs: SENTRY-1676
> https://issues.apache.org/jira/browse/SENTRY-1676
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw 
> RuntimeExceptio
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  146cea2b9467ce82b69bbf402933b1aa350bcd46 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
> 
> 
> Diff: https://reviews.apache.org/r/58093/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 128 (original), 221 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line230>
> >
> > should retries = i + 1?

Looks like 'retries' is only used for reporting. I didn't change the retry code 
at all - we can handle this as a separate refactoring.


> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 129 (original), 222 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line231>
> >
> > should we use "break" to get out of the for() loop instead of setting 
> > "i = retryStrategyMaxRetries;"? In current implementation, the line 226 
> > "retries = i;" will be called when InterruptedException  is called, then 
> > "retries" will be set as "retryStrategyMaxRetries". That is not correct

The existing code isn't very clear but seems to be doing the job. I have not 
touched any of the retry logic - can we handle this separately, if needed?


> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
> > Line 125 (original), 214 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686937#file1686937line276>
> >
> > can we have the test case that the first time, client returns 
> > exception, but the second time, it returns correct result. This is to test 
> > the retry behavior.

There was no original test for this as well. I agree, this would be good to 
test, I suggest doing this separately from this changeset which is specificaly 
for the snapshot creation logic.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58284/#review171443
---


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 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/1/
> 
> 
> Testing
> ---
> 
> Updated the unit test to test for bigger HMS snapshots
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
> > Line 125 (original), 214 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686937#file1686937line276>
> >
> > can we have the test case that the first time, client returns 
> > exception, but the second time, it returns correct result. This is to test 
> > the retry behavior.
> 
> Alexander Kolbasov wrote:
> There was no original test for this as well. I agree, this would be good 
> to test, I suggest doing this separately from this changeset which is 
> specificaly for the snapshot creation logic.

I opened SENTRY-1701 for the improvements of the retry logic and the unit test 
request.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58284/#review171443
-------


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 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/1/
> 
> 
> Testing
> ---
> 
> Updated the unit test to test for bigger HMS snapshots
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171517
---



Why do you want to actually recreate a new instance of HMSFollower every time 
you start a service? The only thing that's actually needed is to manage 
execution of the service, not the instance.

IMO HMSFollower should just be a singleton which is created once and can get 
scheduled or not depending on the need. You don't have to make it a singleton 
as part of this change (this would be nice though) but actually creating a new 
one every time seems to be not needed.

Since HMSFollower doesn't actually keep state - it is all stored in DB it 
doesn't make much sense to have multiple instances. And since it is runnable, 
adding start/stop methods to it doesn't seem right.

- Alexander Kolbasov


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 7, 2017, 5: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
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> > Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
> It is possible. I will add "synchronized" in front of it.
> 
> Alexander Kolbasov wrote:
> Under what circumstances is it possible? If it is possible, it may not be 
> sufficient to add synchronized() - you may need to protect access to the 
> variables as well, but I a not convinced that it is possible. Do we run our 
> tests in parallel?
> 
> Na Li wrote:
> I am not familar with how Sentry service is actually used. 
> Hypathetically, it is possible that several threads hold the same instance 
> and call its close() concurrently.

This should never hapen during regular use - I think this may only happen if 
tests are running in parallel but I guess that it will break many things.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review171666
---


On April 13, 2017, 4:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 13, 2017, 4:47 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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57901/#review172044
---




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment245111>

Since this is execurted as part of the client, the metric part is tricky - 
it depends on the enclosing service metric system.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 54 (patched)
<https://reviews.apache.org/r/57901/#comment245108>

Why do you need to keep it stateful and remember the transport?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 67 (patched)
<https://reviews.apache.org/r/57901/#comment245107>

Why do you need that? You can just the config object to constructor instead



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 137 (patched)
<https://reviews.apache.org/r/57901/#comment245109>

You can just pass appropriate config object in constructor.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 210 (patched)
<https://reviews.apache.org/r/57901/#comment245110>

Why don't you return the created transport here?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 121 (original), 59 (patched)
<https://reviews.apache.org/r/57901/#comment245112>

Calling this in a constructor is suspicious since this is an overridable 
method. You can safeky call it from connect()



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 157 (original), 78 (patched)
<https://reviews.apache.org/r/57901/#comment245113>

It would be better to process all these constants during construction - 
they never change so there is no need to do it on every connect


- Alexander Kolbasov


On April 13, 2017, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 13, 2017, 7:34 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

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58221/#review172104
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 107 (patched)
<https://reviews.apache.org/r/58221/#comment245179>

Please add comment explaining how do you use the future.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 282 (patched)
<https://reviews.apache.org/r/58221/#comment245181>

Please add clarifying parenthesis:

if (notificationLogEnabled && (hmsFollowerExecutor == null) && (hmsFollower 
!= null)) {



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 292 (patched)
<https://reviews.apache.org/r/58221/#comment245183>

We can't handle this - if HMSFollowert should start and it doesn't, 
SentryService should fail. The easiest thing may be just to propagate the 
exception up the chain.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 343 (patched)
<https://reviews.apache.org/r/58221/#comment245184>

You can move this outside the try block and return immediately if 
storeCleanPeriodSecs <= 0. This will simplify the code a bit.


- Alexander Kolbasov


On April 17, 2017, 5:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 17, 2017, 5:55 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 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/12/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58481/#review172553
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 75 (patched)
<https://reviews.apache.org/r/58481/#comment245656>

I don't think we need this



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 270 (patched)
<https://reviews.apache.org/r/58481/#comment245654>

We log failures to process events when we detect these, so is there a real 
need to log it here as well?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 259 (original), 278 (patched)
<https://reviews.apache.org/r/58481/#comment245655>

There is no point in calling this if there are no events


- Alexander Kolbasov


On April 20, 2017, 6:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 20, 2017, 6:14 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58412: SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172585
---


Ship it!




Ship It!

- Alexander Kolbasov


On April 21, 2017, 2:20 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> ---
> 
> (Updated April 21, 2017, 2:20 a.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/persistent/SentryStore.java
>  08520f30f3529af88d7dab9ae3623f28f0e43558 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  1543379eec2403fd1fbe61947c4c38a189177895 
>   
> 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/4/
> 
> 
> Testing
> ---
> 
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 220 (original), 305 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line327>
> >
> > DbTask is returning an empty object mapping, why is this not necessary 
> > with tables?

I should fix this - we should still go over all partitions even if the table 
location is empty.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 361 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line397>
> >
> > do we want to process a DB without a location?

We still want to go over all tables (which we do) but since there is no 
location, there is nothing to emit for the result.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 362 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line398>
> >
> > shouldn't be better to have a singleton for emptyObjectMapping? or we 
> > want different ObjectMapping with empty maps?

Makes sense. Will do.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 314 (original), 415 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line457>
> >
> > why is the 'failOnRetry' not needed anymore? isn't needed?

Failing in this context doesn't make any sense. It could have a value when this 
was part of HMS plugin (even there it is questionable). We are getting state 
from Hive the only result of this setting would be the failure to create the 
initial snapshot and apply subsequent deltas. We can't communicate the failure 
back to hive and at this stage it isn't possible to fix anything.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 317 (original), 416 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line460>
> >
> > are all executors killed when an exception is thrown here? if not, do 
> > we want to kill the current work because of the failure?

The executors are destroyed in the close() method, not here. It is up to the 
caller to call close() even if exception happened.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line463>
> >
> > should it be good to have a new method for this merging code?

I am not sure that it will be helpful - it is 10 lines of code that's not used 
by anything else. The only thing we'll get is a pretty method name. I'd rather 
not do it.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
> > Line 99 (original), 100 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line103>
> >
> > isn't setDefaultSchema a better name? Btw, Is it Scheme or Schema? If 
> > it is for teting, do we want to keep it package-private only?

Will change the name. It can't be package private because the test is in 
different package (sentry-tests/sentry-tests-hive)


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
> > Lines 118 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line121>
> >
> > the method javadoc indicates that null is returned if path is 
> > null/empty." but here we're throwing an exception. Which one is true?

Will update comment.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58284/#review171918
---


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 a.m.)

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

c/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 374 (patched)
<https://reviews.apache.org/r/58221/#comment245001>

catch parameter ie is unused



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 425 (patched)
<https://reviews.apache.org/r/58221/#comment245000>

The code uses synchronized() on both, so the comment isn't quite right - it 
would handle concurrent calls to start()/stop() - not that it is needed.


- Alexander Kolbasov


On April 13, 2017, 7:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 13, 2017, 7:56 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/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58481/#review172380
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 259 (original), 257 (patched)
<https://reviews.apache.org/r/58481/#comment245484>

This still preserves the original problem when events are not processed for 
some reason - you will get a flood of logs. In this case events would be always 
non-empty.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 261 (original), 258 (patched)
<https://reviews.apache.org/r/58481/#comment245482>

This comment no longer matches the code.


- Alexander Kolbasov


On April 19, 2017, 2:43 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 19, 2017, 2:43 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: [DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

Sergio,

thank you for clarifying the Hive version story.

Do we have any explicit policy about compatibility with other components and 
Hive specifically? Do we need to support older versions of Hive or supporting 
the current released Hive 1.x version is sufficient?

At least it seems that we need to address 
SENTRY-1323 Bump the hive version to 1.2.0

to move forward.

- Alex

> On Apr 19, 2017, at 8:34 AM, Sergio Pena <sergio.p...@cloudera.com> wrote:
> 
> Pointing the branch to master sounds good.
> 
> I just have one concern regarding the Hive dependency that Sentry HA needs.
> This new feature requires that the Hive version running with Sentry master
> has HMS notification logs enabled. Hive 1.1.0 (currently officially
> supported by Sentry 1.7) started this HMS notification feature but seems it
> is unstable. At least I found a bug on Hive 1.1.0 that it might make Sentry
> unusable with this version + others required fixes and improvements that
> exist only on Hive 2.x.
> 
> This bug is fixed on Hive 1.2.0:
>   HIVE-9501: DbNotificationListener doesn't include dbname in create
> database notification and does not include tablename in create table
> notification.
> 
> To make things complicated, bumping Sentry to support Hive 1.2.0 is not an
> easy task due to the new authorization v2 hooks that are used in Hive. See
> the comments on the below SENTRY jira:
> 
>   SENTRY-1323: Bump the hive version to 1.2.0
> 
> So, my question is what would happen with the Sentry 1.x compatibility if
> we do this switch from sentry-ha-redesign -> master?
> 
> On Tue, Apr 18, 2017 at 3:45 AM, Colm O hEigeartaigh <cohei...@apache.org>
> wrote:
> 
>> OK thanks, sounds good to me then. Let's try and do development work on
>> master from now on though...
>> 
>> Colm.
>> 
>> On Tue, Apr 18, 2017 at 2:14 AM, Alexander Kolbasov <ak...@cloudera.com>
>> wrote:
>> 
>>> FYI, I contacted Colin Ma who was working on the refactoring and he was
>> Ok
>>> with skipping this change for 1.8. That said, the refactoring was done
>> for
>>> a reason it we should get back to it post 1.8,
>>> 
>>> - Alex
>>> 
>>> On Mon, Apr 17, 2017 at 5:51 PM Hao Hao <hao@cloudera.com> wrote:
>>> 
>>>> The proposal is good to me as well. Should we start a vote on this? Or
>> we
>>>> can just start to do it if no one is objecting?
>>>> 
>>>> Best,
>>>> Hao
>>>> 
>>>> On Mon, Apr 17, 2017 at 4:42 PM, Vamsee Yarlagadda <
>> vam...@cloudera.com>
>>>> wrote:
>>>> 
>>>>>> 
>>>>>> 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 <
>> ak...@cloudera.com
>>>> 
>>>>> 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
>>>>>> 
>>>>>> 
>>>>>>&

Clarification for MPath changes for SENTRY-1587

[Alexander Kolbasov]

Hao,

Can you clarify the changes you propose for MPath class and related
package.jdo changes for SENTRY-1587?

You suggest changing the identity type from "database" to "application",
but the pathId is not initialized in constructor and not assigned anywhere.
What is your intention here?

Also you changed the equals() method to include ID. What kind of equality
do you require - equality by path value or equality by identity? What is
your thinking here?

This is in regards to the code review https://reviews.apache.org/r/58412.

Thanks,

- Alex

Re: [ATTENTION] PMC members

[Alexander Kolbasov]

I am wondering how many people except me are committers but not PMC members?

- Alex

> On Apr 13, 2017, at 3:17 AM, Colm O hEigeartaigh  wrote:
> 
> Hi PMC members of Apache Sentry,
> 
> There is a discussion on the priv...@sentry.apache.org mailing list that
> requires the attention of Sentry PMC members. There appears to be worringly
> few Sentry PMC members who remain active in the project. I'm cc'ing this to
> the dev list in case there are PMC members who have not subscribed to the
> private list.
> 
> Could I ask all PMC members who are still actively monitoring the project
> to let themselves be known on the private list? If you are having a problem
> subscribing to it then drop me a mail directly.
> 
> Colm.
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com