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

2016-07-12 Thread Sravya Tirukkovalur 

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


Ship it!




Ship It!

- Sravya Tirukkovalur


On July 11, 2016, 6:55 p.m., Colin McCabe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49738/
> ---
> 
> (Updated July 11, 2016, 6:55 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 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java
>  434ac41 
> 
> Diff: https://reviews.apache.org/r/49738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin McCabe
> 
>

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

2016-07-11 Thread Hao Hao 

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


Ship it!




Ship It!

- Hao Hao


On July 11, 2016, 6:55 p.m., Colin McCabe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49738/
> ---
> 
> (Updated July 11, 2016, 6:55 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 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java
>  434ac41 
> 
> Diff: https://reviews.apache.org/r/49738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin McCabe
> 
>

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

2016-07-11 Thread Colin McCabe 


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 161
> > 
> >
> > Exception will be thrown if create table failed?

yes


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 222
> > 
> >
> > Should it be expected? It seems it should never happen?

Good question.  It is not a harmful condition, so there is no reason to 
consider it an error.  It could happen if a daemon loses the leader election 
but then regains the leadership shortly thereafter.


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java,
> >  line 234
> > 
> >
> > Yeah, not clear about what is the function for? It seems not be called 
> > anywhere?

see above comment-- the purpose is to verify that the fencing table still 
exists as part of a transaction.  There will be callers later


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 175
> > 
> >
> > Could you document a bit more why do we need these two properties to 
> > disallow operations outside of transcations? Will it ever happen?

This is an existing setting of SentryStore-- nothing is being changed here, 
just moving around some code.  See the section about "non-transactional reads" 
here: 
http://www.datanucleus.org/products/datanucleus/jdo/performance_tuning.html to 
see why we don't want them


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java,
> >  line 156
> > 
> >
> > Why do we need to removeth haContext?

HAContext was used by the old HA implementation which is being replaced.  There 
is no function for it any more.


> On July 11, 2016, 9:17 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java,
> >  line 36
> > 
> >
> > Should we add a e2e test for fencing between active and standby?

I definitely agree that more tests would be good.  We have that on the roadmap, 
and shouldn't block this patch on that, though.


- Colin


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


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

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

2016-07-11 Thread Colin McCabe 

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

(Updated July 11, 2016, 6:55 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 (updated)
-

  
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 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java
 434ac41 

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


Testing
---


Thanks,

Colin McCabe

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

2016-07-11 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?
> 
> Colin McCabe wrote:
> 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
> 
> Sravya Tirukkovalur wrote:
> Thanks for the info Colin! Yes, I agree that we should commit for read 
> only transactions as well, and hence I was curious why there is no commit. 
> But rather we just have 
> 
> if (tx.isActive()) {
> tx.rollback();
>   }
>   
> 
> Where it will always be active, as we have not made an attempt to commit.

Good point.  Let me add the tx#commit


- Colin


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


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

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

2016-07-11 Thread Hao Hao 

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




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


Exception will be thrown if create table failed?



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


Should it be expected? It seems it should never happen?



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


Yeah, not clear about what is the function for? It seems not be called 
anywhere?



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


Could you document a bit more why do we need these two properties to 
disallow operations outside of transcations? Will it ever happen?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 


Why do we need to removeth haContext?



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


Should we add a e2e test for fencing between active and standby?


- Hao Hao


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

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 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 49738: SENTRY-1317: Implement fencing required for active/passive

2016-07-07 Thread Colin McCabe 

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

  
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 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java
 434ac41 

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


Testing
---


Thanks,

Colin McCabe