Re: Review Request 50474: SENTRY-1404: Use the new INodeAttributesProvider API in sentry-hdfs

2016-07-29 Thread Anne Yu 

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


Ship it!




Ship It!

- Anne Yu


On July 29, 2016, 7:41 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50474/
> ---
> 
> (Updated July 29, 2016, 7:41 p.m.)
> 
> 
> Review request for sentry and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Use the new INodeAttributesProvider API in sentry-hdfs
> 
> 
> Diffs
> -
> 
>   pom.xml a4f2bcc14855a2e5f712f9c56fbdb1e137edfa93 
>   sentry-binding/sentry-binding-solr/pom.xml 
> 8b94c87118841c2ce775c3e79ff6bb43ba5185fd 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/hadoop/hdfs/server/namenode/AuthorizationProvider.java
>  383d64d88c74c93cd79ccb189301b9c1a6d919e4 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationConstants.java
>  883680182fd01f4b0b1a7fb5d8eb4d7f23f5851e 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java
>  f639f5fbacc85b2adb757087c2eba66b092743a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/MockSentryAuthorizationProvider.java
>  2085b525db0b7a9fda77a4be62026da3bf74b7ac 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java
>  5da0dc2fb4a47098bd5f9b36dbacb0296162f95b 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  60085b20774fb28c978f60b97bbef0ecbadb8d8f 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  25d13d62a1a16beba16ae46fbd28f2490fca10bc 
> 
> Diff: https://reviews.apache.org/r/50474/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 50611: Sentry HA Test: programmatic failover, validate privileges before and afterwards.

2016-07-29 Thread Anne Yu 

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

(Updated July 29, 2016, 8:03 p.m.)


Review request for sentry, Hao Hao and Sravya Tirukkovalur.


Changes
---

1. addressed Rahul's comments;
2. enable sentry client pool for testing;
3. ensure can parse external test configurations and internally specified test 
values.


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


Repository: sentry


Description
---

Sentry HA Test: test programmatic failover, validate privileges before and 
afterwards. validate expected exception is thrown.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestProgrammaticFailover.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestSentryHABase.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 7dc3d0f30583b9edd16015218b199e298d192e8c 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 054b1935975368db058791102f3892f027e31636 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
 dac11517f36a743dff8730465fbf2397ebf16c88 

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


Testing
---


Thanks,

Anne Yu

Re: Review Request 50182: Sentry-1371: Rework Sentry start up and Hive state fetch

2016-07-29 Thread Sravya Tirukkovalur 

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



Seems like we need to not block Sentry startup for the sake of HMS full update 
(to avoid dependencies at startup), but would like to not serve requests ( 
eventually only for Hive requests) until the full update processing is finished?


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


Seems like we should make a synchronized call to createIntialUpdate anyways 
as the calling thread needs to block? In which case, is there a reason why we 
want to sppin up another thread?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 257 - 266)


Seems like we should never apply deltas unless full update processing is 
completed?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 269 - 272)


Seems like this needs to be implemented partially in this patch? That is, 
use fullUpdateComplete? Eventually we should set this value based on 
notification id stored in Sentry?


- Sravya Tirukkovalur


On July 29, 2016, 1:34 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50182/
> ---
> 
> (Updated July 29, 2016, 1:34 a.m.)
> 
> 
> Review request for sentry, Anne Yu and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added logic in HMSFollower for hive full update. 
> Block sentry service from start up until full update fetch is complete.
> Fetch notification logs which are before failover but did not applied.
> 
> 
> Diffs
> -
> 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> e767e06c13be2312ccd04e39c518ddef4fbf7744 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.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/SentryService.java
>  e93e5b45caa1db61d6c5d69312f66f475dd02ee8 
> 
> Diff: https://reviews.apache.org/r/50182/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 50474: SENTRY-1404: Use the new INodeAttributesProvider API in sentry-hdfs

2016-07-29 Thread Hao Hao 

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

(Updated July 29, 2016, 7:41 p.m.)


Review request for sentry and Sravya Tirukkovalur.


Repository: sentry


Description
---

Use the new INodeAttributesProvider API in sentry-hdfs


Diffs (updated)
-

  pom.xml a4f2bcc14855a2e5f712f9c56fbdb1e137edfa93 
  sentry-binding/sentry-binding-solr/pom.xml 
8b94c87118841c2ce775c3e79ff6bb43ba5185fd 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/hadoop/hdfs/server/namenode/AuthorizationProvider.java
 383d64d88c74c93cd79ccb189301b9c1a6d919e4 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationConstants.java
 883680182fd01f4b0b1a7fb5d8eb4d7f23f5851e 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java
 f639f5fbacc85b2adb757087c2eba66b092743a2 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
 PRE-CREATION 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/MockSentryAuthorizationProvider.java
 2085b525db0b7a9fda77a4be62026da3bf74b7ac 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java
 5da0dc2fb4a47098bd5f9b36dbacb0296162f95b 
  
sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 60085b20774fb28c978f60b97bbef0ecbadb8d8f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 25d13d62a1a16beba16ae46fbd28f2490fca10bc 

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


Testing
---


Thanks,

Hao Hao

Re: Review Request 50578: Sentry-1411: The sentry client should retry RPCs if it gets a SentryStandbyException

2016-07-29 Thread Sravya Tirukkovalur 


> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java,
> >  lines 273-278
> > 
> >
> > It is not very clear what we are trying to achieve here. Seems like the 
> > order of endpoints matter as per this logic?
> 
> Hao Hao wrote:
> Here is after reach the retry max, checking for all exception received is 
> it caused by standby or active is unreachable. The order of endpoints does 
> not matter.

I think we should log.error these exceptions, even if we throw only the last 
exception. As connection to each could have failed due to a different reason. 
And can we make the exception messages a bit more descriptive to make it more 
actionable. Basically, if we have unreachable exceptions, then there is some 
thing weird going on like either a kerberos issue or service name is wrong etc. 
On the other hand if there is no active, even if there are standbys, then it 
can mean passive is taking too long to become active. We should also come back 
to this once we support {ACTIVE, STANDBY, STARTING} states.


- Sravya


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


On July 29, 2016, 12:47 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> ---
> 
> (Updated July 29, 2016, 12:47 a.m.)
> 
> 
> Review request for sentry and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The sentry client should retry configurable times RPCs if it gets a 
> SentryStandbyException
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java
>  acb906fc8b792db70dc09821c0a6ad7cf7361807 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  1039e6ebfde32ec8db82287458b8b4b6a23136a3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  a35bf1d4781c7424283610c7fa67ede990e35fef 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  48ee66a4132113d49dc16c419bd496dd1572b59d 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java
>  3a38b243e79e007c0e15ee947828301136608525 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java
>  51bba31bddc4404506bbe42f9f8d444b36d5056c 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java
>  15eab15b79f9057d173e0ea4ea64e152ade698d6 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50578/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 50611: Sentry HA Test: programmatic failover, validate privileges before and afterwards.

2016-07-29 Thread Anne Yu 


> On July 29, 2016, 6:26 p.m., Rahul Sharma wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestProgrammaticFailover.java,
> >  lines 117-120
> > 
> >
> > how is the boolean return value from failover() useful?

Yeah, will validate this boolean value before move forward to validation.


> On July 29, 2016, 6:26 p.m., Rahul Sharma wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestSentryHABase.java,
> >  line 36
> > 
> >
> > Logger for this class?

Good catch, will modify this to the current class.


- Anne


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


On July 29, 2016, 5:27 p.m., Anne Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50611/
> ---
> 
> (Updated July 29, 2016, 5:27 p.m.)
> 
> 
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
> 
> 
> Bugs: SENTRY-1423
> https://issues.apache.org/jira/browse/SENTRY-1423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry HA Test: test programmatic failover, validate privileges before and 
> afterwards. validate expected exception is thrown.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestProgrammaticFailover.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestSentryHABase.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  7dc3d0f30583b9edd16015218b199e298d192e8c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  054b1935975368db058791102f3892f027e31636 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  dac11517f36a743dff8730465fbf2397ebf16c88 
> 
> Diff: https://reviews.apache.org/r/50611/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anne Yu
> 
>

Re: Review Request 50611: Sentry HA Test: programmatic failover, validate privileges before and afterwards.

2016-07-29 Thread Rahul Sharma 

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestProgrammaticFailover.java
 (lines 117 - 120)


how is the boolean return value from failover() useful?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestSentryHABase.java
 (line 36)


Logger for this class?


- Rahul Sharma


On July 29, 2016, 5:27 p.m., Anne Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50611/
> ---
> 
> (Updated July 29, 2016, 5:27 p.m.)
> 
> 
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
> 
> 
> Bugs: SENTRY-1423
> https://issues.apache.org/jira/browse/SENTRY-1423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry HA Test: test programmatic failover, validate privileges before and 
> afterwards. validate expected exception is thrown.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestProgrammaticFailover.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestSentryHABase.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  7dc3d0f30583b9edd16015218b199e298d192e8c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  054b1935975368db058791102f3892f027e31636 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  dac11517f36a743dff8730465fbf2397ebf16c88 
> 
> Diff: https://reviews.apache.org/r/50611/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anne Yu
> 
>