Review Request 47084: SENTRY-1233: Logging improvements to SentryConfigToolSolr

2016-05-06 Thread Gregory Chanan 

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

Review request for sentry and Vamsee Yarlagadda.


Repository: sentry


Description
---

>From working with this tool for a bit, I think the following additions would 
>be useful:
1) The tool warns/errors on cased role names being converted to lower case, but 
still prints the cased name in the log. For clarity these should also be lower 
cased.
2) When not importing, we should still print a message about what we would do, 
i.e. a dry-run message.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java
 22895eb938f8c9092d68472038ddbf93534594f5 

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


Testing
---

Manually tested, verified that the message is printed to the log.


Thanks,

Gregory Chanan

Re: Review Request 46909: SENTRY-1229: Add caching to SentryGenericProviderBackend.

2016-05-06 Thread Gregory Chanan 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 (line 90)


It's not great how the code (and bugs, see below) with 
SimpleFileProviderBackend is essentially duplicated here.  Here is one way this 
would ideally look (not subject to compatibility constraits).

1) Define a "Cache" interface with a single function: getCache
2) Split out the getPrivileges / getRoles implementations from 
SimpleFileProviderBackend to something like CacheProviderBackend that takes a 
Cache object
3) SimpleFileProviderBackend then uses a CacheProviderBackend that always 
returns the table built in initialize
4) The Generic backend, when caching is enabled, uses a 
CacheProviderBackend that is identical to your cache here.  I.e. the Cache here 
just implements getCache in the way get() is implemented.

5) Ideally, the cache-enabled and non-cache enabled backends here would be 
two completely separate types.  The usual way of accomplishing that is via a 
factory; I don't know if that's feasible because I think the configuration 
specifies the actual type (not a factory type) and calls the constructor 
directly.  If you think a factory is a better option, I don't think you need to 
implement it now, but at least file a jira.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 (line 95)


hmm...the SimpleFileProviderBackend doesn't do this either?  Do you know 
what's up with that?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 (line 211)


with this implementation, all the requests at the beginning are going to 
fail, right?  Should we just pause while we build the cache initially?  Slower 
startup/responses seem desirible than everything initially failing.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 21)


Comment about what this is useful for?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 28)


just make this a constructor param (or builder)?  Don't see the need to 
change this in the middle.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 31)


the other files I checked use LoggerFactory.getLogger -- is there a 
difference?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 33)


this isn't called externally afaict.  Why have it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 50)


aren't these units not matching?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 80)


What if it fails?  At some point you probably want to just start denying 
everything?  because revokes would be completely broken.  Maybe make that 
configurable?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
 (line 89)


I'm not familiar with scheduleAtFixedRate, but do you actually need to 
sleep here?  Can you just run the function and it will be called again at the 
fixed period?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 (line 30)


I would just collapse this and Updatable.  It's not obvious the distinction 
will be useful going forward and if it is, we can always split it out later.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 (line 74)


just put this in the constructor



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 (line 86)


just put this in the constructor and make final

Re: [ANNOUNCE] New Sentry Committers - Li Li and Colm O hEigeartaigh

2016-05-06 Thread Sravya Tirukkovalur 
Well deserved! Congratulations Colm and Li! And thanks for your
contributions so far! Looking forward to your continued contributions!

On Fri, May 6, 2016 at 10:25 AM, Lenni Kuff  wrote:

> I'm excited to announce that the Apache Sentry PMC has voted to invite Li
> Li and Colm O hEigeartaigh as committers on the Apache Sentry project.
>
> Please join me in congratulating Li and Colm!
>
> Thanks,
> Lenni
>



-- 
Sravya Tirukkovalur

Review Request 47075: SENTRY-1208: Make HOST implied in privileges if not specified explicitly.

2016-05-06 Thread Ashish Singh 

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

Review request for sentry, Colin Ma and Dapeng Sun.


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


Repository: sentry


Description
---

SENTRY-1208: Make HOST implied in privileges if not specified explicitly.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/KafkaTSentryPrivilegeConvertor.java
 af73755ae92ad4f61243d18b2521d144189532b4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellKafka.java
 d49bc57b5c0b619e5e0e34fcf1f2676535c9e13c 

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


Testing
---

Extended existing unit tests.


Thanks,

Ashish Singh

Re: [ANNOUNCE] New Sentry Committers - Li Li and Colm O hEigeartaigh

2016-05-06 Thread Ashish Singh 
Congrats Li and Colm!

On Fri, May 6, 2016 at 10:37 AM, Chao Sun  wrote:

> Congrats Li and Colm!
>
> On Fri, May 6, 2016 at 10:25 AM, Lenni Kuff  wrote:
>
> > I'm excited to announce that the Apache Sentry PMC has voted to invite Li
> > Li and Colm O hEigeartaigh as committers on the Apache Sentry project.
> >
> > Please join me in congratulating Li and Colm!
> >
> > Thanks,
> > Lenni
> >
>



-- 

Regards,
Ashish

Re: Review Request 46909: SENTRY-1229: Add caching to SentryGenericProviderBackend.

2016-05-06 Thread Ashish Singh 


> On May 6, 2016, 8:39 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java,
> >  line 62
> > 
> >
> > TSentryPrivilege is generated by *.thrift file, we could not edit it 
> > directly.

Missed that, will move it along with filtering code.


- Ashish


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


On May 5, 2016, 9:44 p.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46909/
> ---
> 
> (Updated May 5, 2016, 9:44 p.m.)
> 
> 
> Review request for sentry, Dapeng Sun, Gregory Chanan, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Bugs: SENTRY-1229
> https://issues.apache.org/jira/browse/SENTRY-1229
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1229: Add caching to SentryGenericProviderBackend.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
>  ffd3af4903dd59f37ea1ff4a55138742fdfa74da 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
>  2c2ff351fc26b4a4df36678d843bb85c29e3d234 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
>  222b6fd530a0e26ea625d1be0fd68b0828558316 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
>  60502895a0fcdd781f5bd61b29a676a7c96f81b8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  dce3dade7f42fe35a849612c5caf2e98d2dac578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  00e3fbde76ebf84704ee110adfca30869845a7b8 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
>  a2cfa28da2af548a872ec0d2e5620ae1c27041b3 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java
>  250522ed33394a667b1a0d6c47c68b210e0214a3 
> 
> Diff: https://reviews.apache.org/r/46909/diff/
> 
> 
> Testing
> ---
> 
> Extended Kafka e2e tests. Will be running perf tests for Kafka-Sentry 
> integration with caching turned on.
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 46909: SENTRY-1229: Add caching to SentryGenericProviderBackend.

2016-05-06 Thread Ashish Singh 


> On May 6, 2016, 8:34 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java,
> >  line 123
> > 
> >
> > For the filter, some situations should be considered:
> > eg, db=db1->table=*, with the current implementation, it will be ignore 
> > and the authorization will be failed.
> > Just return all the privileges for user, and do the filter as the 
> > improvement.

Makes sense as this is anyway coming from cache. Will have to amend tests 
accordingly. Created SENTRY-1245 to track this.


> On May 6, 2016, 8:34 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java,
> >  line 91
> > 
> >
> > If Sentry client has to get all metadatas from server side, I think 
> > there should be one RPC call for this.
> > Too many rpc calls are in loadFromRemote() if there has  many roles, 
> > this will be a performance issue. SENTRY-197 is the feature about 
> > import/export for Hive, and it can be a reference to add a new interface to 
> > get all these metadatas.

You are right, however given that the fetch happens in background the perf hit 
should not affect end users. Filed a follow up jira, SENTRY-1246, to track 
this. Makes sense?


- Ashish


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


On May 5, 2016, 9:44 p.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46909/
> ---
> 
> (Updated May 5, 2016, 9:44 p.m.)
> 
> 
> Review request for sentry, Dapeng Sun, Gregory Chanan, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Bugs: SENTRY-1229
> https://issues.apache.org/jira/browse/SENTRY-1229
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1229: Add caching to SentryGenericProviderBackend.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
>  ffd3af4903dd59f37ea1ff4a55138742fdfa74da 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
>  2c2ff351fc26b4a4df36678d843bb85c29e3d234 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
>  222b6fd530a0e26ea625d1be0fd68b0828558316 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
>  60502895a0fcdd781f5bd61b29a676a7c96f81b8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  dce3dade7f42fe35a849612c5caf2e98d2dac578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  00e3fbde76ebf84704ee110adfca30869845a7b8 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
>  a2cfa28da2af548a872ec0d2e5620ae1c27041b3 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java
>  250522ed33394a667b1a0d6c47c68b210e0214a3 
> 
> Diff: https://reviews.apache.org/r/46909/diff/
> 
> 
> Testing
> ---
> 
> Extended Kafka e2e tests. Will be running perf tests for Kafka-Sentry 
> integration with caching turned on.
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 45550: Fix some "major" issues identified by Sonarqube

2016-05-06 Thread Colm O hEigeartaigh 

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

(Updated May 6, 2016, 10:28 a.m.)


Review request for sentry.


Repository: sentry


Description
---

SENTRY-1168: Fix some "major" issues identified by Sonarqube


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java
 5238414 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java
 f3799ca 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatFactory.java
 d2c6072 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 775a1f5 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
 4ef86e6 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java
 217b7b3 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
 0d1db89 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 8e70492 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java
 9e08571 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
 d741c44 
  
sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/SentrySqoopError.java
 b86c59f 
  
sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBindingSingleton.java
 39e001f 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 
1ccf7de 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryUserException.java
 9a8f85d 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryVersionInfo.java
 53fe3af 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
 d024032 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java
 99cefb7 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java
 3a05a3b 
  
sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/IndexerConstants.java
 2182525 
  
sentry-core/sentry-core-model-indexer/src/main/java/org/apache/sentry/core/model/indexer/IndexerModelAuthorizables.java
 d15e911 
  
sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java
 9f76bda 
  
sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizables.java
 c3292c7 
  
sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/Job.java
 e7f43ef 
  
sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/Link.java
 53194ea 
  
sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/Server.java
 8d86a38 
  
sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/SqoopActionConstant.java
 2b867fa 
  
sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/SqoopModelAuthorizables.java
 11ce7ec 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 80a25aa 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
 3203ecd 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 a091f71 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 c791ab3 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
 58aa10d 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 1fdf418 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java
 782367a 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
 5ff7294 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/hadoop/hdfs/server/namenode/AuthorizationProvider.java
 114dbb0 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationConstants.java
 ea1514c 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 c2416c1 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java
 c701723

Re: Review Request 46909: SENTRY-1229: Add caching to SentryGenericProviderBackend.

2016-05-06 Thread Dapeng Sun 

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




sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
 (line 62)


TSentryPrivilege is generated by *.thrift file, we could not edit it 
directly.



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


*ENABLE_CACHING_DEFAULT* should on the behind of *ENABLE_CACHING*, same as 
*CACHING_TTL_MS_DEFAULT*



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


sentry.service.client.cache.enabled


- Dapeng Sun


On 五月 6, 2016, 5:44 a.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46909/
> ---
> 
> (Updated 五月 6, 2016, 5:44 a.m.)
> 
> 
> Review request for sentry, Dapeng Sun, Gregory Chanan, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Bugs: SENTRY-1229
> https://issues.apache.org/jira/browse/SENTRY-1229
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1229: Add caching to SentryGenericProviderBackend.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
>  ffd3af4903dd59f37ea1ff4a55138742fdfa74da 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
>  2c2ff351fc26b4a4df36678d843bb85c29e3d234 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
>  222b6fd530a0e26ea625d1be0fd68b0828558316 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
>  60502895a0fcdd781f5bd61b29a676a7c96f81b8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  dce3dade7f42fe35a849612c5caf2e98d2dac578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  00e3fbde76ebf84704ee110adfca30869845a7b8 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
>  a2cfa28da2af548a872ec0d2e5620ae1c27041b3 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java
>  250522ed33394a667b1a0d6c47c68b210e0214a3 
> 
> Diff: https://reviews.apache.org/r/46909/diff/
> 
> 
> Testing
> ---
> 
> Extended Kafka e2e tests. Will be running perf tests for Kafka-Sentry 
> integration with caching turned on.
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 46909: SENTRY-1229: Add caching to SentryGenericProviderBackend.

2016-05-06 Thread Colin Ma 

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




sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
 (line 34)


This is generated by thrift tool, it shouldn't be updated, or will be a 
problem for the thrift api change in the future.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 (line 20)


It's better not to use "*" in import



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 (line 122)


For the filter, some situations should be considered:
eg, db=db1->table=*, with the current implementation, it will be ignore and 
the authorization will be failed.
Just return all the privileges for user, and do the filter as the 
improvement.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 (line 91)


If Sentry client has to get all metadatas from server side, I think there 
should be one RPC call for this.
Too many rpc calls are in loadFromRemote() if there has  many roles, this 
will be a performance issue. SENTRY-197 is the feature about import/export for 
Hive, and it can be a reference to add a new interface to get all these 
metadatas.


- Colin Ma


On May 5, 2016, 9:44 p.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46909/
> ---
> 
> (Updated May 5, 2016, 9:44 p.m.)
> 
> 
> Review request for sentry, Dapeng Sun, Gregory Chanan, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Bugs: SENTRY-1229
> https://issues.apache.org/jira/browse/SENTRY-1229
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1229: Add caching to SentryGenericProviderBackend.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
>  ffd3af4903dd59f37ea1ff4a55138742fdfa74da 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TSentryPrivilege.java
>  2c2ff351fc26b4a4df36678d843bb85c29e3d234 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
>  222b6fd530a0e26ea625d1be0fd68b0828558316 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
>  60502895a0fcdd781f5bd61b29a676a7c96f81b8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  dce3dade7f42fe35a849612c5caf2e98d2dac578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  00e3fbde76ebf84704ee110adfca30869845a7b8 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
>  a2cfa28da2af548a872ec0d2e5620ae1c27041b3 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java
>  250522ed33394a667b1a0d6c47c68b210e0214a3 
> 
> Diff: https://reviews.apache.org/r/46909/diff/
> 
> 
> Testing
> ---
> 
> Extended Kafka e2e tests. Will be running perf tests for Kafka-Sentry 
> integration with caching turned on.
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 47012: SENTRY-1222: Improve SentryIniPolicyFileFormatter to support user section in .ini file

2016-05-06 Thread Dapeng Sun 

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


Ship it!




Ship It!

- Dapeng Sun


On 五月 5, 2016, 9:59 p.m., Colin Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47012/
> ---
> 
> (Updated 五月 5, 2016, 9:59 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry support grant role to user, for the import/export with the .ini file, 
> the user section should be supported.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
>  06fe1fe 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
>  0e7ee3d 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/PolicyFileConstants.java
>  dfe4fe0 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  a52ad8f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceImportExport.java
>  2665db1 
> 
> Diff: https://reviews.apache.org/r/47012/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin Ma
> 
>