Re: Review Request 49777: SENTRY-1321: Implement HMSFollower in Sentry service which reads the NotificationLog entries

2016-07-08 Thread Sravya Tirukkovalur 

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

(Updated July 9, 2016, 2:13 a.m.)


Review request for sentry and Hao Hao.


Repository: sentry


Description (updated)
---

- Adding HMSFollower and changes in Sentryservice to use it.
- Test changes - start sentryservice before hive
- Moving HiveAuthzConf to sentry-binding-hive-conf, so that sentry-provider-db 
does not have to depend on sentry-binding-hive-commong which creates a circular 
dependency
- Moving messaging/json to sentry-binding-hive-follower to avoid circular 
dependency between sentry-hive-binding and sentry-provider-db


Diffs (updated)
-

  sentry-binding/pom.xml 30bca146786a833391fa44c62e036777b694a7b1 
  sentry-binding/sentry-binding-hive-common/pom.xml 
18b422d5a688e636af4e01b382fa3e5677ac884b 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 ad19b3754527e25c6509571a47f3e31a077b9e56 
  sentry-binding/sentry-binding-hive-conf/pom.xml PRE-CREATION 
  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 PRE-CREATION 
  sentry-binding/sentry-binding-hive-follower/pom.xml PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java
 PRE-CREATION 
  sentry-provider/sentry-provider-db/pom.xml 
b8143ffa3adca9e47e7cb092131d65064d57c86b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
 f54f161b381088285486a5ca74972f93ee620547 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 809af064a8f54860ae796f1d9f29fd8f52568663 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 0ab8192a86178f4febcec7384ebc3a5be0cc69fb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 767bcbe02e0d511f52bc869e7b6a1ee1e6584a5a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 2c4948e9d100f2cf0cb5b7772489194c62b8a857 

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


Testing
---

Testing not complete yet. Submmitting a preview of changes for review as this 
work also is important for SENTRY-1371


Thanks,

Sravya Tirukkovalur

Re: Review Request 49397: SENTRY-1329: Adapt SentryMetaStorePostEventListener to write HMS notification logs

2016-07-08 Thread Sravya Tirukkovalur 

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

(Updated July 8, 2016, 9:30 p.m.)


Review request for sentry, Anne Yu, Colin McCabe, Hao Hao, and Nachiket Vaidya.


Bugs: Sentry-1329
https://issues.apache.org/jira/browse/Sentry-1329


Repository: sentry


Description
---

This patch adapts SentryMetaStorePostEventListener to write HMS notification 
logs. Also,
1. Implements the SentryJSONMessageFactory to add custom information in the 
notification log entry message, which includes
 1.1. Implementing Message class for each message type
 1.2. Implementing a deserializer
2. Implements JSONAlterPartitionMessage and JSONAlterTableMessage to work 
around the issue in Hive 1.1.0.
3. Testing:
 3.1. Sentry functionality: TestSentryListenerSentryDeserializer to verify 
functionality using Sentry's SentryMetastorePostEventListener and Sentry 
Notification log deserializer.
 3.2. TestDbNotificationListenerSentryDeserializer uses Hive's 
DbNotificationListener and Sentry's JSON deserializeri. This would make sure 
Sentry is able to read the Notification logs written by Hive's 
DBNotificationListener
 3.3. TestSentryListenerInBuiltDeserializer uses Sentry's 
SentryMetastorePostEventListener and Hive's inbuilt Notification log 
deserializer: This would make sure Sentry is not breaking other users of 
NotificationLog who might be using Hive's in built serializer



Change-Id: I680beb6db4e534bb0a9e6ee042ea0d4f33f0943f

update

Change-Id: I6ce8846a801b3effc6a851ab3ce6f866b4a0c23e


Diffs (updated)
-

  sentry-binding/sentry-binding-hive/pom.xml 
07aaae3bac34582fe8222ff166a52c1e208b223a 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 d12ac151ddbf35d612fee3f869d602d6cdf54aa8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java
 PRE-CREATION 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 767bcbe02e0d511f52bc869e7b6a1ee1e6584a5a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 2c4948e9d100f2cf0cb5b7772489194c62b8a857 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 b72e317621610eababf5d01886bc00dca9f5e7ec 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestDbNotificationListenerSentryDeserializer.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestHMSNotificationLogUsingDBNotificationListener.java
 0b328d402d005d6f858e87fbe64eedd5f9f1a092 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerInBuiltDeserializer.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java
 PRE-CREATION 

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


Testing
---

Added new tests


Thanks,

Re: Review Request 49397: SENTRY-1329: Adapt SentryMetaStorePostEventListener to write HMS notification logs

2016-07-08 Thread Sravya Tirukkovalur 


> On July 8, 2016, 6:29 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 81
> > 
> >
> > Why 99 here?

Good question. I tried to retain some little details from 
DbNotificationListener. This is one of those: 
https://github.com/apache/hive/blob/4cbc4a652abe87742ccb4ea93e9ddfadce7d3e94/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java#L72
Seems like the "id" is never used. Will add a comment here based on my limited 
hive knowledge.


> On July 8, 2016, 6:29 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 152
> > 
> >
> > Why 0L for the eventID and 0 for the eventTime?

To signify it is of long type.


> On July 8, 2016, 6:29 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 280
> > 
> >
> > Could we do processing here to extract the needed location change 
> > information based on the variant?

Actually thinking a bit more about it. Seems like we will need dbname, 
tablename, location for both newTable and oldTable in all those cases. It is 
just that they might be duplicates in some cases. Adding new validations in the 
next patch.


> On July 8, 2016, 6:29 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 423
> > 
> >
> > Why not use the same unit for ttl and METASTORE_EVENT_DB_LISTENER_TTL?

This is another thing which I borrowed from DbNotificationListener. We could 
change it but might be better to keep it consistent unless we know for sure, as 
we do intend to move back?


> On July 8, 2016, 6:29 p.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 435
> > 
> >
> > Should we make it configurable?

Same as above.


- Sravya


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


On July 7, 2016, 9:57 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49397/
> ---
> 
> (Updated July 7, 2016, 9:57 p.m.)
> 
> 
> Review request for sentry, Anne Yu, Colin McCabe, Hao Hao, and Nachiket 
> Vaidya.
> 
> 
> Bugs: Sentry-1329
> https://issues.apache.org/jira/browse/Sentry-1329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch adapts SentryMetaStorePostEventListener to write HMS notification 
> logs. Also,
> 1. Implements the SentryJSONMessageFactory to add custom information in the 
> notification log entry message, which includes
>  1.1. Implementing Message class for each message type
>  1.2. Implementing a deserializer
> 2. Implements JSONAlterPartitionMessage and JSONAlterTableMessage to work 
> around the issue in Hive 1.1.0.
> 3. Testing:
>  3.1. Sentry functionality: TestSentryListenerSentryDeserializer to verify 
> functionality using Sentry's SentryMetastorePostEventListener and Sentry 
> Notification log deserializer.
>  3.2. TestDbNotificationListenerSentryDeserializer uses Hive's 
> DbNotificationListener and Sentry's JSON deserializeri. This would make sure 
> Sentry is able to read the Notification logs written by Hive's 
> DBNotificationListener
>  3.3. TestSentryListenerInBuiltDeserializer uses Sentry's 
> SentryMetastorePostEventListener and Hive's inbuilt Notification log 
> deserializer: This would make sure Sentry is not breaking other users of 
> NotificationLog who might be using Hive's in built serializer
> 
> 
> 
> Change-Id: I680beb6db4e534bb0a9e6ee042ea0d4f33f0943f
> 
> update
> 
> Change-Id: I6ce8846a801b3effc6a851ab3ce6f866b4a0c23e
> 
> 
> Diffs
> -
> 
>   sentry-binding/sentry-binding-hive/pom.xml 
> 07aaae3bac34582fe8222ff166a52c1e208b223a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  d12ac151ddbf35d612fee3f869d602d6cdf54aa8 
>   
>

Re: Review Request 49738: SENTRY-1317: Implement fencing required for active/passive

2016-07-08 Thread Sravya Tirukkovalur 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 114)


Signature does not throw an exception



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 154)


Signature does not throw an exception



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 201)


Do we want to instead rethrow the exception, so that the caller is aware 
that operation did not succeed?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 234)


Seems like this function is basically executing select* . What is the 
purpose of it?


- Sravya Tirukkovalur


On July 8, 2016, 7:59 p.m., Colin McCabe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49738/
> ---
> 
> (Updated July 8, 2016, 7:59 p.m.)
> 
> 
> Review request for sentry, Hao Hao, Rahul Sharma, and Sravya Tirukkovalur.
> 
> 
> Bugs: SENTRY-1317
> https://issues.apache.org/jira/browse/SENTRY-1317
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1317: Implement fencing required for active/passive
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  3da4906 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PluginCacheSyncUtil.java
>  4ce16c7 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  5bf2f6e 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestHAUpdateForwarder.java
>  5246e05 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
>  cacc29f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7dad496 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceRegister.java
>  79dfe48 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  3de1f65 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activators.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java
>  e846766 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java
>  80a6571 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  809af06 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  0ab8192 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java
>  f14b586 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java
>  799d5ef 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  3ef1cf7 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
>  3ff97df 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java
>  a8e8a03 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  cb2d9c9 
>   
>

Re: Review Request 49738: SENTRY-1317: Implement fencing required for active/passive

2016-07-08 Thread Colin McCabe 


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 135
> > 
> >
> > Why rollback? What does roll back mean for read only queries? Is it 
> > still recommended to commit the transaction?

While it is true that this is a read-only transaction, rollback is still 
required to terminate the transaction.  Otherwise, later we will receive the 
error "javax.jdo.JDOUserException: Transaction is still active.  You should 
always close your transactions correctly using commit() or rollback()."  See 
this link for more information:  
http://stackoverflow.com/questions/4418317/jdo-exception-in-google-app-engine-transaction


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 174
> > 
> >
> > Rollback if it has'nt been successful?

Good point.  I made this a transaction.


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 206
> > 
> >
> > The purpose of this function does not look accurate?

renamed


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java,
> >  line 70
> > 
> >
> > We do not have support for sql server in Sentry. So best to leave it 
> > out for now?

ok


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 171
> > 
> >
> > Excecute within a transaction?

It is fine to execute queries within a transaction.


> On July 8, 2016, 6:04 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java,
> >  line 992
> > 
> >
> > It is unclear to me what new validations are we adding to this test?

It's not adding new validations, just supplying the Activator object which the 
SentryStore now needs.


- Colin


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


On July 7, 2016, 9:33 p.m., Colin McCabe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49738/
> ---
> 
> (Updated July 7, 2016, 9:33 p.m.)
> 
> 
> Review request for sentry, Hao Hao, Rahul Sharma, and Sravya Tirukkovalur.
> 
> 
> Bugs: SENTRY-1317
> https://issues.apache.org/jira/browse/SENTRY-1317
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1317: Implement fencing required for active/passive
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  3da4906 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PluginCacheSyncUtil.java
>  4ce16c7 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  5bf2f6e 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestHAUpdateForwarder.java
>  5246e05 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
>  cacc29f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7dad496 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceRegister.java
>  79dfe48 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java
>  PRE-CREATION 
>   
>

Re: Review Request 49397: SENTRY-1329: Adapt SentryMetaStorePostEventListener to write HMS notification logs

2016-07-08 Thread Hao Hao 


> On July 1, 2016, 12:52 a.m., Hao Hao wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java,
> >  line 66
> > 
> >
> > What if the notification log fails, but the DDL operations succeeds as 
> > discussed in HIVE-13966?
> 
> Sravya Tirukkovalur wrote:
> Yes, Hive-13966 is needed for both Sentry's listener as well as 
> DBNotificationListener. There is no way to workaround that without it just 
> from with in the Listener.

I see, would be better to add this explicity in the comment?


- Hao


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


On July 7, 2016, 9:57 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49397/
> ---
> 
> (Updated July 7, 2016, 9:57 p.m.)
> 
> 
> Review request for sentry, Anne Yu, Colin McCabe, Hao Hao, and Nachiket 
> Vaidya.
> 
> 
> Bugs: Sentry-1329
> https://issues.apache.org/jira/browse/Sentry-1329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch adapts SentryMetaStorePostEventListener to write HMS notification 
> logs. Also,
> 1. Implements the SentryJSONMessageFactory to add custom information in the 
> notification log entry message, which includes
>  1.1. Implementing Message class for each message type
>  1.2. Implementing a deserializer
> 2. Implements JSONAlterPartitionMessage and JSONAlterTableMessage to work 
> around the issue in Hive 1.1.0.
> 3. Testing:
>  3.1. Sentry functionality: TestSentryListenerSentryDeserializer to verify 
> functionality using Sentry's SentryMetastorePostEventListener and Sentry 
> Notification log deserializer.
>  3.2. TestDbNotificationListenerSentryDeserializer uses Hive's 
> DbNotificationListener and Sentry's JSON deserializeri. This would make sure 
> Sentry is able to read the Notification logs written by Hive's 
> DBNotificationListener
>  3.3. TestSentryListenerInBuiltDeserializer uses Sentry's 
> SentryMetastorePostEventListener and Hive's inbuilt Notification log 
> deserializer: This would make sure Sentry is not breaking other users of 
> NotificationLog who might be using Hive's in built serializer
> 
> 
> 
> Change-Id: I680beb6db4e534bb0a9e6ee042ea0d4f33f0943f
> 
> update
> 
> Change-Id: I6ce8846a801b3effc6a851ab3ce6f866b4a0c23e
> 
> 
> Diffs
> -
> 
>   sentry-binding/sentry-binding-hive/pom.xml 
> 07aaae3bac34582fe8222ff166a52c1e208b223a 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  d12ac151ddbf35d612fee3f869d602d6cdf54aa8 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterPartitionMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/JSONAlterTableMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAddPartitionMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterTableMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropDatabaseMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropTableMessage.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageDeserializer.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java
>  PRE-CREATION 
>   
>

Re: Review Request 49526: SENTRY-1365

2016-07-08 Thread Sravya Tirukkovalur 

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



Nice work Hao! Just one comment about the length of path.


sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql 
(line 15)


We use Varchar(4000) for URIs, are you sure 255 would be sufficient?


- Sravya Tirukkovalur


On July 5, 2016, 11:54 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49526/
> ---
> 
> (Updated July 5, 2016, 11:54 p.m.)
> 
> 
> Review request for sentry and Anne Yu.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1365: Upgrading SQL script for HMSPaths persistence
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 6d08b5c0bf29a2c33f0baf5701c3efaf4b5d29d3 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> d522026e18e6c2532487e9d4a988e408e6736008 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> d27d7b93cd3fb50d54c14c4eddee1ce59c785b13 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> ced5c31a6a1d12dec88c39132797cf89dfa36d0f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  6d563324bdf631790edce512502394b8fa81bd1f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  927f302f789913a787b177b89af8b90a23c013ca 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-1.7.0-to-1.8.0.sql
>  fbe2dc8e285c2459c4563cda7156ce83bdf66a0a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-1.7.0-to-1.8.0.sql
>  f0df187ab3299f0c87bb26eb32bee0001f45fc5a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-1.7.0-to-1.8.0.sql
>  f1666be9244ada88c59107f564d7c7662299f851 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-1.7.0-to-1.8.0.sql
>  b39292d621ad68ff62f2585372a58c28e0eb8d37 
> 
> Diff: https://reviews.apache.org/r/49526/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 49738: SENTRY-1317: Implement fencing required for active/passive

2016-07-08 Thread Sravya Tirukkovalur 

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



Nice work Colin! Left some initial feedback. Seems like we need to add testing 
to make sure fencing is being done?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 105)


Document which exceptions are thrown in what conditions?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 120)


SentryAccessDeniedException does not seem like the right exception we want 
to throw here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 126)


Same as above, is this the right exception type?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 135)


Why rollback? What does roll back mean for read only queries? Is it still 
recommended to commit the transaction?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 171)


Excecute within a transaction?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 174)


Rollback if it has'nt been successful?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
 (line 206)


The purpose of this function does not look accurate?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java
 (line 70)


We do not have support for sql server in Sentry. So best to leave it out 
for now?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java
 (line 992)


It is unclear to me what new validations are we adding to this test?


- Sravya Tirukkovalur


On July 7, 2016, 9:33 p.m., Colin McCabe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49738/
> ---
> 
> (Updated July 7, 2016, 9:33 p.m.)
> 
> 
> Review request for sentry, Hao Hao, Rahul Sharma, and Sravya Tirukkovalur.
> 
> 
> Bugs: SENTRY-1317
> https://issues.apache.org/jira/browse/SENTRY-1317
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1317: Implement fencing required for active/passive
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  3da4906 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PluginCacheSyncUtil.java
>  4ce16c7 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  5bf2f6e 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestHAUpdateForwarder.java
>  5246e05 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
>  cacc29f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7dad496 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceRegister.java
>  79dfe48 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  3de1f65 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activators.java
>  PRE-CREATION 
>   
>

Re: Review Request 49526: SENTRY-1365

2016-07-08 Thread Anne Yu 

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


Ship it!




Ship It!

- Anne Yu


On July 5, 2016, 11:54 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49526/
> ---
> 
> (Updated July 5, 2016, 11:54 p.m.)
> 
> 
> Review request for sentry and Anne Yu.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1365: Upgrading SQL script for HMSPaths persistence
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 6d08b5c0bf29a2c33f0baf5701c3efaf4b5d29d3 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> d522026e18e6c2532487e9d4a988e408e6736008 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> d27d7b93cd3fb50d54c14c4eddee1ce59c785b13 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> ced5c31a6a1d12dec88c39132797cf89dfa36d0f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  6d563324bdf631790edce512502394b8fa81bd1f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  927f302f789913a787b177b89af8b90a23c013ca 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-1.7.0-to-1.8.0.sql
>  fbe2dc8e285c2459c4563cda7156ce83bdf66a0a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-1.7.0-to-1.8.0.sql
>  f0df187ab3299f0c87bb26eb32bee0001f45fc5a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-1.7.0-to-1.8.0.sql
>  f1666be9244ada88c59107f564d7c7662299f851 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-1.7.0-to-1.8.0.sql
>  b39292d621ad68ff62f2585372a58c28e0eb8d37 
> 
> Diff: https://reviews.apache.org/r/49526/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>