Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Nov. 9, 2016, 3:34 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description --- Since currently non pool model is used for sentry clients, here update retry logic for non-pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 730bfec98a78ac11f1fae8ab84f9e6715e802a40 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java b7d2be153576f80aa1c0fd230d29523cf6a047c3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java a2499040d77dbeb9925a529063fd6a492dd57cc4 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java 3f57a003903961a6aea98bd583a14b65bd2b98a2 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Nov. 9, 2016, 3:33 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description --- Since currently non pool model is used for sentry clients, here update retry logic for non-pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 730bfec98a78ac11f1fae8ab84f9e6715e802a40 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java b7d2be153576f80aa1c0fd230d29523cf6a047c3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java a2499040d77dbeb9925a529063fd6a492dd57cc4 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java 3f57a003903961a6aea98bd583a14b65bd2b98a2 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
> On Nov. 4, 2016, 10:10 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java, > > line 123 > > <https://reviews.apache.org/r/52526/diff/6/?file=1554520#file1554520line123> > > > > It is possible that two threads call this concurrently, both discover a > > problem, one of them reconnects first and another one will close already > > reconnected client and try to connect it again which isn't good, a simple solution is to make invokeImpl synchronized - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review155003 ----------- On Nov. 4, 2016, 6:26 p.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > --- > > (Updated Nov. 4, 2016, 6:26 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Since currently non pool model is used for sentry clients, here update retry > logic for non-pool model. For each full retry we will cycle through all > available sentry servers randomly. After each full retry, we will have a > small random sleep. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 4f42a51b1449fe15f856ba252103e66383e175d7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 3a96d0b124c00efc99cef256c72c25f5c6168007 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 730bfec98a78ac11f1fae8ab84f9e6715e802a40 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > b7d2be153576f80aa1c0fd230d29523cf6a047c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > a2499040d77dbeb9925a529063fd6a492dd57cc4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 5b0e12bbf12510d8d424aa2b7f51076a913234c5 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java > 3f57a003903961a6aea98bd583a14b65bd2b98a2 > > Diff: https://reviews.apache.org/r/52526/diff/ > > > Testing > --- > > Tested in my dev with the following cases: > 1. one server is down before client connection > 2. one server is down after client connection and during client request > > > Thanks, > > Li Li > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Nov. 4, 2016, 6:26 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description --- Since currently non pool model is used for sentry clients, here update retry logic for non-pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 730bfec98a78ac11f1fae8ab84f9e6715e802a40 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java b7d2be153576f80aa1c0fd230d29523cf6a047c3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java a2499040d77dbeb9925a529063fd6a492dd57cc4 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java 3f57a003903961a6aea98bd583a14b65bd2b98a2 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
> On Nov. 4, 2016, 6:30 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java, > > line 74 > > <https://reviews.apache.org/r/52526/diff/5/?file=1551855#file1551855line74> > > > > Can this code be executed concurrently? Will it handle isCOnnected > > logic correctly in this case? > > > > I think it is better to put this logic for if(!isConnected) > > connectWithRetry inside client which is synchronized. Since connectWithRetry is synchronized and client will be closed in client.connectWithRetry(), the code can be executed concurrently. I will put the logic inside client class and call client.close() outside when got connection problem. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review154867 ----------- On Nov. 2, 2016, 6:08 p.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > --- > > (Updated Nov. 2, 2016, 6:08 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Since currently non pool model is used for sentry clients, here update retry > logic for non-pool model. For each full retry we will cycle through all > available sentry servers randomly. After each full retry, we will have a > small random sleep. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 4f42a51b1449fe15f856ba252103e66383e175d7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 3a96d0b124c00efc99cef256c72c25f5c6168007 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 730bfec98a78ac11f1fae8ab84f9e6715e802a40 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > b7d2be153576f80aa1c0fd230d29523cf6a047c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > a2499040d77dbeb9925a529063fd6a492dd57cc4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 5b0e12bbf12510d8d424aa2b7f51076a913234c5 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java > 3f57a003903961a6aea98bd583a14b65bd2b98a2 > > Diff: https://reviews.apache.org/r/52526/diff/ > > > Testing > --- > > Tested in my dev with the following cases: > 1. one server is down before client connection > 2. one server is down after client connection and during client request > > > Thanks, > > Li Li > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review154927 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 74) <https://reviews.apache.org/r/52526/#comment224726> will put it inside client sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 75) <https://reviews.apache.org/r/52526/#comment224722> agree. will update sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 80) <https://reviews.apache.org/r/52526/#comment224718> no, only connectWithRetry() can throw IOException. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 81) <https://reviews.apache.org/r/52526/#comment224719> Ooops, I intend to retry (see the comment), but forgot to change the code. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 89) <https://reviews.apache.org/r/52526/#comment224723> either way is fine for me. it is very easy to ignore the '!' in if clause. I will keep the current one for now. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 95) <https://reviews.apache.org/r/52526/#comment224717> we can add a debug log here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 96) <https://reviews.apache.org/r/52526/#comment224716> because it is originally TTransportException. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 100) <https://reviews.apache.org/r/52526/#comment224713> If changed to Exception, all current sentry client has to change the catch exception cod, e.g. impala. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 126) <https://reviews.apache.org/r/52526/#comment224721> actually client will be closed in cient.connectWithRetry(). But I will move it to the client - Li Li On Nov. 2, 2016, 6:08 p.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > --- > > (Updated Nov. 2, 2016, 6:08 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Since currently non pool model is used for sentry clients, here update retry > logic for non-pool model. For each full retry we will cycle through all > available sentry servers randomly. After each full retry, we will have a > small random sleep. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 4f42a51b1449fe15f856ba252103e66383e175d7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 3a96d0b124c00efc99cef256c72c25f5c6168007 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 730bfec98a78ac11f1fae8ab84f9e6715e802a40 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > b7d2be153576f80aa1c0fd230d29523cf6a047c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > a2499040d77dbeb9925a529063fd6a492dd57cc4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 5b0e12bbf12510d8d424aa2b7f51076a913234c5 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java > 3f57a003903961a6aea98bd583a14b65bd2b98a2 > > Diff: https://reviews.apache.org/r/52526/diff/ > > > Testing > --- > > Tested in my dev with the following cases: > 1. one server is down before client connection > 2. one server is down after client connection and during client request > > > Thanks, > > Li Li > >
Re: Review Request 52916: SENTRY-1505 CommitContext isn't used by anything and should be removed
> On Nov. 3, 2016, 9:59 p.m., Hao Hao wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java,
> > line 88
> > <https://reviews.apache.org/r/52916/diff/3/?file=1552764#file1552764line88>
> >
> > Make it void here since we are not returning anything? Same thing for
> > the rest of APIs.
>
> Alexander Kolbasov wrote:
> I had to do it this way because of the way Mockito works - it needs an
> object returned. This only affects testig path, not normal code.
Nit: you can use
doThrow(SomeException).when(mockedObject).methodReturningVoid(...); instead of
when(mockedObject.methodReturningObject(...)).thenThrow(someException) to
handle the returning void method in Mockito.
e.g. void createRole():
doThrow(new SentryAlreadyExistsException("Role: " + roleName + " already
exists")).when(mockStore).createRole(Matchers.anyString(), roleName,
Matchers.anyString());
We can create a followup jira for it.
- Li
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52916/#review154821
---
On Nov. 3, 2016, 3:58 a.m., Alexander Kolbasov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52916/
> ---
>
> (Updated Nov. 3, 2016, 3:58 a.m.)
>
>
> Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, Li Li, and
> Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> SENTRY-1505 CommitContext isn't used by anything and should be removed
>
>
> Diffs
> -
>
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> f09004226124d3c2290abf7e36bc09b46afb8223
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
> 687a7e0443a6c47cbf39f9eaff7e796636963970
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandler.java
> e0a5f03d5cc72dadbd0325efd7e9734a4bf48558
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandlerInvoker.java
> 1d9c246214e50cea08b8c69df9a1c6a3080346ed
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
> e59d12a6188d84d1a6bd627dd7776cefa0fbea44
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CommitContext.java
> c74dbf3be18c68aa8fdaca2ce12c24e7e8ae5a54
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> 17113e62f54f4758e4c86bccbedfa6f57b830e4a
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandler.java
> b1a4b7fb39af3db1181a125365da09655694c480
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandlerInvoker.java
> 856ef9a6a830ab6829d55cfe45a8e7f15620b81b
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
> 5dff12a5026ebb6764e83baf90c7b12d069701fa
>
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java
> 8b3599fd9cb4dd80700c6bf405d35db16a72e3d7
>
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestNotificationHandlerInvoker.java
> 6a2f48f39246efbb5d7e80b4562ea19240dfd8b6
>
> Diff: https://reviews.apache.org/r/52916/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 52916: SENTRY-1505 CommitContext isn't used by anything and should be removed
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52916/#review154809 --- LGTM. fix it and ship it sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 253) <https://reviews.apache.org/r/52916/#comment224516> remove this line? - Li Li On Nov. 3, 2016, 3:58 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52916/ > --- > > (Updated Nov. 3, 2016, 3:58 a.m.) > > > Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, Li Li, and > Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1505 CommitContext isn't used by anything and should be removed > > > Diffs > - > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > f09004226124d3c2290abf7e36bc09b46afb8223 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > 687a7e0443a6c47cbf39f9eaff7e796636963970 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandler.java > e0a5f03d5cc72dadbd0325efd7e9734a4bf48558 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/NotificationHandlerInvoker.java > 1d9c246214e50cea08b8c69df9a1c6a3080346ed > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java > e59d12a6188d84d1a6bd627dd7776cefa0fbea44 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CommitContext.java > c74dbf3be18c68aa8fdaca2ce12c24e7e8ae5a54 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 17113e62f54f4758e4c86bccbedfa6f57b830e4a > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandler.java > b1a4b7fb39af3db1181a125365da09655694c480 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandlerInvoker.java > 856ef9a6a830ab6829d55cfe45a8e7f15620b81b > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 5dff12a5026ebb6764e83baf90c7b12d069701fa > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java > 8b3599fd9cb4dd80700c6bf405d35db16a72e3d7 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestNotificationHandlerInvoker.java > 6a2f48f39246efbb5d7e80b4562ea19240dfd8b6 > > Diff: https://reviews.apache.org/r/52916/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Nov. 2, 2016, 6:08 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Changes --- Addressed Alexander's comments Repository: sentry Description --- Since currently non pool model is used for sentry clients, here update retry logic for non-pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 730bfec98a78ac11f1fae8ab84f9e6715e802a40 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java b7d2be153576f80aa1c0fd230d29523cf6a047c3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java a2499040d77dbeb9925a529063fd6a492dd57cc4 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java 3f57a003903961a6aea98bd583a14b65bd2b98a2 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52526/#review154454
---
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
(line 214)
<https://reviews.apache.org/r/52526/#comment224012>
I see. sure.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
(line 235)
<https://reviews.apache.org/r/52526/#comment224013>
sure. I will removing the sleep and add a todo here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
(line 49)
<https://reviews.apache.org/r/52526/#comment224014>
i see. sure.
- Li Li
On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
>
> (Updated Oct. 27, 2016, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya
> Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> Since currently non pool model is used for sentry clients, here update retry
> logic for non-pool model. For each full retry we will cycle through all
> available sentry servers randomly. After each full retry, we will have a
> small random sleep.
>
>
> Diffs
> -
>
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
> 4f42a51b1449fe15f856ba252103e66383e175d7
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
> 3a96d0b124c00efc99cef256c72c25f5c6168007
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
> 730bfec98a78ac11f1fae8ab84f9e6715e802a40
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
> b7d2be153576f80aa1c0fd230d29523cf6a047c3
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> f98ebd135a383ff090b1b204cc62644403310df7
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
> 5b0e12bbf12510d8d424aa2b7f51076a913234c5
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
> 3f57a003903961a6aea98bd583a14b65bd2b98a2
>
> Diff: https://reviews.apache.org/r/52526/diff/
>
>
> Testing
> ---
>
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 52582: SENTRY-1489: Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52582/#review154180 --- Ship it! Ship It! - Li Li On Oct. 28, 2016, 8:03 p.m., Anne Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52582/ > --- > > (Updated Oct. 28, 2016, 8:03 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya > Tirukkovalur. > > > Bugs: SENTRY-1489 > https://issues.apache.org/jira/browse/SENTRY-1489 > > > Repository: sentry > > > Description > --- > > org.apache.sentry.tests.e2e.dbprovider.TestDbCrossOperations and some other > tests occasionally fail with time out exception. Should make a more flexible > rule for different type of tests. By nature, some tests just take longer time > to finish. > > > Diffs > - > > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > e7cccbfb7955a497f485e3b69e9dc4d9db191473 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbCrossOperations.java > 60812cfa82d318ddfad32e439d3895f49ed21180 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithHiveServer.java > a314c0dde9c60e7dbb51156cea13980bf8600bf1 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 62da0256e8546a25f0ed0a97b8f3c21eec4fecaa > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/SlowE2ETest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52582/diff/ > > > Testing > --- > > > Thanks, > > Anne Yu > >
Re: Review Request 52582: SENTRY-1489: Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52582/#review154156 --- LGTM. Only one issue I am not sure: is it ok to resolve two jiras in the same patch? sentry-tests/sentry-tests-solr/pom.xml (line 83) <https://reviews.apache.org/r/52582/#comment223651> I am not sure if it is good to resolve two jiras in a same patch. Maybe better make this change in a separate patch? - Li Li On Oct. 24, 2016, 11:18 p.m., Anne Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52582/ > --- > > (Updated Oct. 24, 2016, 11:18 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya > Tirukkovalur. > > > Bugs: SENTRY-1489 > https://issues.apache.org/jira/browse/SENTRY-1489 > > > Repository: sentry > > > Description > --- > > org.apache.sentry.tests.e2e.dbprovider.TestDbCrossOperations and some other > tests occasionally fail with time out exception. Should make a more flexible > rule for different type of tests. By nature, some tests just take longer time > to finish. > > > Diffs > - > > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > e7cccbfb7955a497f485e3b69e9dc4d9db191473 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbCrossOperations.java > 60812cfa82d318ddfad32e439d3895f49ed21180 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithHiveServer.java > a314c0dde9c60e7dbb51156cea13980bf8600bf1 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 62da0256e8546a25f0ed0a97b8f3c21eec4fecaa > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/SlowE2ETest.java > PRE-CREATION > sentry-tests/sentry-tests-solr/pom.xml > a60b4eed4dde6c182b061681305013b3bd548cc3 > > Diff: https://reviews.apache.org/r/52582/diff/ > > > Testing > --- > > > Thanks, > > Anne Yu > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Oct. 27, 2016, 4:41 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description (updated) --- Since currently non pool model is used for sentry clients, here update retry logic for non-pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 730bfec98a78ac11f1fae8ab84f9e6715e802a40 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java b7d2be153576f80aa1c0fd230d29523cf6a047c3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java f98ebd135a383ff090b1b204cc62644403310df7 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java 3f57a003903961a6aea98bd583a14b65bd2b98a2 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 53175: SENTRY-1501: Add option to use non pool model for sentry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53175/ --- (Updated Oct. 25, 2016, 10:25 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Currently sentry clients e.g. impala uses non pool model (SentryPolicyServiceClientDefaultImpl), thus it's better to keep the non pool model for those clients to avoid unnecessary incompatible issues. Also the current pool model (PoolClientInvocationHandler) does not manage the opening connections very well. e.g. Opening connections with failed servers should be closed promptly. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 842d5cafb06910fcbe6c53002f2101ec5b890a9e sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 9e90af8f5b638d346e3f48405441ad97b9ef09ad sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java d1ac44766b049fb6fb6e6ce822cc0b63c0f6d66e Diff: https://reviews.apache.org/r/53175/diff/ Testing --- Thanks, Li Li
Re: Review Request 53175: SENTRY-1501: Add option to use non pool model for sentry client
> On Oct. 25, 2016, 10:06 p.m., Hao Hao wrote: > > Thanks Li! The patch looks good. Could you please comment why current pool > > model does not manage the opening connections very well. sure. I will add a comment - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53175/#review153827 --- On Oct. 25, 2016, 9:51 p.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53175/ > --- > > (Updated Oct. 25, 2016, 9:51 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, > and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Currently sentry clients e.g. impala uses non pool model > (SentryPolicyServiceClientDefaultImpl), thus it's better to keep the non pool > model for those clients to avoid unnecessary incompatible issues. > Also the current pool model (PoolClientInvocationHandler) does not manage the > opening connections very well. e.g. Opening connections with failed servers > should be closed promptly. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > 9e90af8f5b638d346e3f48405441ad97b9ef09ad > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java > d1ac44766b049fb6fb6e6ce822cc0b63c0f6d66e > > Diff: https://reviews.apache.org/r/53175/diff/ > > > Testing > --- > > > Thanks, > > Li Li > >
Re: Review Request 53175: SENTRY-1501: Add option to use non pool model for sentry client
> On Oct. 25, 2016, 9:07 p.m., Anne Yu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java, > > line 35 > > <https://reviews.apache.org/r/53175/diff/1/?file=1545318#file1545318line35> > > > > This is very nice. Maybe we can make e2e tests use pool based clients > > by default. Now we want to use the original model - non pool model which is used by current sentry clients (e.g. impala), so our e2e tests should focused in non pool as well. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53175/#review153822 ------- On Oct. 25, 2016, 8:20 p.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53175/ > --- > > (Updated Oct. 25, 2016, 8:20 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, > and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Currently sentry clients e.g. impala uses non pool model > (SentryPolicyServiceClientDefaultImpl), thus it's better to keep the non pool > model for those clients to avoid unnecessary incompatible issues. > Also the current pool model (PoolClientInvocationHandler) does not manage the > opening connections very well. e.g. Opening connections with failed servers > should be closed promptly. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > 9e90af8f5b638d346e3f48405441ad97b9ef09ad > > Diff: https://reviews.apache.org/r/53175/diff/ > > > Testing > --- > > > Thanks, > > Li Li > >
Review Request 53175: SENTRY-1501: Add option to use non pool model for sentry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53175/ --- Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Currently sentry clients e.g. impala uses non pool model (SentryPolicyServiceClientDefaultImpl), thus it's better to keep the non pool model for those clients to avoid unnecessary incompatible issues. Also the current pool model (PoolClientInvocationHandler) does not manage the opening connections very well. e.g. Opening connections with failed servers should be closed promptly. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 9e90af8f5b638d346e3f48405441ad97b9ef09ad Diff: https://reviews.apache.org/r/53175/diff/ Testing --- Thanks, Li Li
Re: Review Request 53038: SENTRY-1507
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53038/#review153408
---
pom.xml (line 510)
<https://reviews.apache.org/r/53038/#comment222712>
how about setting datanucleus-jdo.version in pom perperty and use
${datanucleus-jdo.version} here?
sentry-binding/sentry-binding-hive-v2/pom.xml (line 35)
<https://reviews.apache.org/r/53038/#comment222713>
The change in line 146 is unnecessary, since datanucleus-jdo.version is
already set to 3.2.0-m3 here.
- Li Li
On Oct. 20, 2016, 12:21 a.m., Alexander Kolbasov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53038/
> ---
>
> (Updated Oct. 20, 2016, 12:21 a.m.)
>
>
> Review request for sentry, Colm O hEigeartaigh, Colin Ma, Hao Hao, Li Li, and
> Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1507
> https://issues.apache.org/jira/browse/SENTRY-1507
>
>
> Repository: sentry
>
>
> Description
> ---
>
> SENTRY-1507
>
>
> Diffs
> -
>
> pom.xml 4eb7d5f5814d65dfdf8cfda2e4ff0f964955f7c9
> sentry-binding/sentry-binding-hive-v2/pom.xml
> e6df18ab70245f1b014a739cd81621357774c278
> sentry-service/sentry-service-server/pom.xml
> 97bd326b0305303194a965a60b6afd0feefbbe0f
> sentry-tests/sentry-tests-kafka/pom.xml
> 4c0ae777f721315bb3e08081e20a4322abb45a72
> sentry-tests/sentry-tests-solr/pom.xml
> a60b4eed4dde6c182b061681305013b3bd548cc3
> sentry-tests/sentry-tests-sqoop/pom.xml
> f70c5c326a4cf3e98fab60dd9c74f8d2144ea4a3
>
> Diff: https://reviews.apache.org/r/53038/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 52888: NPE in log4j.properties parsing
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52888/#review152697 --- Ship it! Ship It! - Li Li On Oct. 14, 2016, 4:08 p.m., Colm O hEigeartaigh wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52888/ > --- > > (Updated Oct. 14, 2016, 4:08 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-1504 > https://issues.apache.org/jira/browse/SENTRY-1504 > > > Repository: sentry > > > Description > --- > > There is a NPE if the log4j.properties configuration file does not contain > "log.threshold". > > > Diffs > - > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > d321531 > > Diff: https://reviews.apache.org/r/52888/diff/ > > > Testing > --- > > > Thanks, > > Colm O hEigeartaigh > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Oct. 14, 2016, 5:06 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description (updated) --- Since currently only pool model is used, here update retry logic for pool model. For each full retry we will cycle through all available sentry servers randomly. After each full retry, we will have a small random sleep. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 842d5cafb06910fcbe6c53002f2101ec5b890a9e sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java f98ebd135a383ff090b1b204cc62644403310df7 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 Diff: https://reviews.apache.org/r/52526/diff/ Testing (updated) --- Tested in my dev with the following cases: 1. one server is down before client connection 2. one server is down after client connection and during client request Thanks, Li Li
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review152359 --- Ship it! Ship It! - Li Li On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Oct. 12, 2016, 7:15 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 89892924839df8058ea82e7819973d576420f578 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 9e9358b8bdcfb4177d0320da26739d990d287f09 > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52675: Create a sentry scale test tool to add various objects and privileges into Sentry and HMS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52675/#review152274 --- sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java (line 33) <https://reviews.apache.org/r/52675/#comment221341> Why make it non final? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java (line 60) <https://reviews.apache.org/r/52675/#comment221217> how about just using: val = System.getProperty(hiveVar, new Configuration().get(hiveVar)) ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java (line 69) <https://reviews.apache.org/r/52675/#comment221221> seems it may not be system property, see line 63? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 42) <https://reviews.apache.org/r/52675/#comment221223> run -> running ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 294) <https://reviews.apache.org/r/52675/#comment221347> db ++ -> db++ ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 309) <https://reviews.apache.org/r/52675/#comment221348> tb ++ -> tb++ ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/TestTools.java (line 68) <https://reviews.apache.org/r/52675/#comment221345> do we need to add 'else' for all other cases (print help)? Better ask someone else to do a code review since I am not familiar with test framework. - Li Li On Oct. 11, 2016, 4:29 a.m., Anne Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52675/ > --- > > (Updated Oct. 11, 2016, 4:29 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya > Tirukkovalur. > > > Bugs: SENTRY-1497 > https://issues.apache.org/jira/browse/SENTRY-1497 > > > Repository: sentry > > > Description > --- > > Specify the scale numbers like databases, tables, views, partitions, columns, > uris, privileges, role, and groups in a config file, the tool can create such > volume of data in Sentry and HMS databases. To speed up test, it can also do > the task parallelly. > > > Diffs > - > > sentry-tests/sentry-tests-hive/pom.xml > a2512ee3919a3958425f4ab74b178d02e0402315 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java > 90713b1aaa688808859e670c8799f8e5be2d6d26 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/TestTools.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/sentry_scale_test_config.xml > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/scripts/scale-test/create-many-dbs-tables.sh > dcdddeb95a896ca8470d0b994f5460531e34d113 > > Diff: https://reviews.apache.org/r/52675/diff/ > > > Testing > --- > > > Thanks, > > Anne Yu > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> On Oct. 11, 2016, 6:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > line 77 > > <https://reviews.apache.org/r/52138/diff/6/?file=1525949#file1525949line77> > > > > I think we should set fullUpdateComplete to static, so that when the > > last HMSFollower update it to true, the next HMSFollower won't do full > > update again. oops, I think I was wrong. You are right. For the ScheduledExecutorService, we do only create HMSFollower once and recall run method. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review152171 --- On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Oct. 6, 2016, 10:22 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 89892924839df8058ea82e7819973d576420f578 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 9e9358b8bdcfb4177d0320da26739d990d287f09 > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review152171 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 77) <https://reviews.apache.org/r/52138/#comment221011> I think we should set fullUpdateComplete to static, so that when the last HMSFollower update it to true, the next HMSFollower won't do full update again. - Li Li On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Oct. 6, 2016, 10:22 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 89892924839df8058ea82e7819973d576420f578 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 9e9358b8bdcfb4177d0320da26739d990d287f09 > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> On Sept. 24, 2016, 11:59 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, > > lines 215-216 > > <https://reviews.apache.org/r/52138/diff/3/?file=1509069#file1509069line215> > > > > fullUpdateComplete variable is only used inside HMSFollower class - I > > would consider moving it inside and making it private. Since it is accessed > > by a single executor thread only there is no need to make it atomic > > (although this wouldn't hurt). > > Hao Hao wrote: > I do not think we shoud change from atomic to non-atomic. > fullUpdateComplete is a shared flag between main thread and HMSFollower. Once > fullUpdateComplete is true, which means full update is complete, HMSFollower > should not retrieve full update again. > > Alexander Kolbasov wrote: > Keeping it atomic is fine. But it seems that fullUpdateComplete isn't > actually shared - it is only used by HMSFollower thread. That's why I > suggested moving the variable inside HMSFollower class. > > Hao Hao wrote: > I see, will change it accordingly. actually fullUpdateComplete is also used in SentryService as a centralized place to share with all HMSFollower threads. Though HMSFollowers are not sharing it at the same time as only 1 HMSFollower is running at certain time, it is a state shared by all HMSFollower threads. The new HMSFollower thread can decide whether need a full update or not, according to the value of fullUpdateComplete updated by the last HMSFollower thread. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150322 --- On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > ------- > > (Updated Oct. 6, 2016, 10:22 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 89892924839df8058ea82e7819973d576420f578 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 9e9358b8bdcfb4177d0320da26739d990d287f09 > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52675: Create a sentry scale test tool to add various objects and privileges into Sentry and HMS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52675/#review152076 --- sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 43) <https://reviews.apache.org/r/52675/#comment220786> better to add a space after '//' sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 84) <https://reviews.apache.org/r/52675/#comment220787> shall we print out configFileFullPath here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 91) <https://reviews.apache.org/r/52675/#comment220788> shall we add 'per' in the config names to make them more clear? Like AVG_VIEWS_PER_DATABASE sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 102) <https://reviews.apache.org/r/52675/#comment220798> what's the meaning of managed table? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 119) <https://reviews.apache.org/r/52675/#comment220791> since KEYTAB_LOCATION is hdfs keytab, we'd better add hdfs in the config name. Or we can add another config for the run as user which is hdfs by default sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 136) <https://reviews.apache.org/r/52675/#comment220790> better to log the EXT_TEST_DATA_PATH, since we will overwrite the dir in line137 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 143) <https://reviews.apache.org/r/52675/#comment220792> remove or update the comment? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 145) <https://reviews.apache.org/r/52675/#comment220789> KEYTAB_LOCATION maybe null or empty, we'd better do some checks before using it sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 152) <https://reviews.apache.org/r/52675/#comment220793> do we need to check HS2_SSL_TRUST_STORE and HS2_SSL_TRUST_STORE_PWD is not null or empty here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 178) <https://reviews.apache.org/r/52675/#comment220796> seems all exception is caught and we can remove SQLException here. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 195) <https://reviews.apache.org/r/52675/#comment220795> shall we throw exception instread of return hs2Conn here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 197) <https://reviews.apache.org/r/52675/#comment220794> shall we call statement.close() in finally when there is exception? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 208) <https://reviews.apache.org/r/52675/#comment220797> what is seq? what if NUM_OF_DATABASES_PER_THREAD - seq - 1 == 0? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 243) <https://reviews.apache.org/r/52675/#comment220800> use fileSystem.create(extPath, false) instead? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 255) <https://reviews.apache.org/r/52675/#comment220801> shall we log the exception here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 280) <https://reviews.apache.org/r/52675/#comment220802> use a final member var for column s1 here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java (line 298) <https://reviews.apache.org/r/52675/#comment220803> remove the space between i and '++'? Also in other for loops in the function. - Li Li On Oct. 10, 2016, 6:14 a.m., Anne Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52675/ > ----------- > > (Updated Oct. 10, 2016, 6:14 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya > Tirukko
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review152003 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 178) <https://reviews.apache.org/r/52526/#comment220693> this is one of the constructor in this class. Do you mean add rand as a memeber var in this class? but rand is only used in this constructor. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java (line 118) <https://reviews.apache.org/r/52526/#comment220702> yes. Good point! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 146) <https://reviews.apache.org/r/52526/#comment220707> There are two different modes for clients, PoolClientInvocationHandler is used for pool mode, while SentryPolicyServiceClientDefaultImpl is for non-pool mode. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 178) <https://reviews.apache.org/r/52526/#comment220711> There are two modes for clients and we may not combine the retry logic together. Here in the pool mode, all the availble connections will be added in a pool, which will be shared by multiple threads. And all threads will share the state of the connections, which is curFreshestEndpointIdx and endpoints in the code. If thread A failed to connect to nth server, then thread B after A will skip n and try n+1. - Li Li On Oct. 7, 2016, 12:30 a.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > --- > > (Updated Oct. 7, 2016, 12:30 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Add retry logic for non-pool model. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 4f42a51b1449fe15f856ba252103e66383e175d7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 3a96d0b124c00efc99cef256c72c25f5c6168007 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 842d5cafb06910fcbe6c53002f2101ec5b890a9e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 5b0e12bbf12510d8d424aa2b7f51076a913234c5 > > Diff: https://reviews.apache.org/r/52526/diff/ > > > Testing > --- > > In non-pool model, for each full retry we will cycle through all available > sentry servers. Before each full retry, we will shuffle the server list, and > after each full retry, we will have a small random sleep. > > > Thanks, > > Li Li > >
Re: Review Request 52582: SENTRY-1489: Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52582/#review151832 --- sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java (line 29) <https://reviews.apache.org/r/52582/#comment220405> It is better to add some some comment for the new class and vars in it. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java (line 44) <https://reviews.apache.org/r/52582/#comment220414> also 'description != null' ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java (line 45) <https://reviews.apache.org/r/52582/#comment220415> add debug log for the SlowE2ETest case showing the timeout used? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java (line 47) <https://reviews.apache.org/r/52582/#comment220416> use final static var for all the timeout? - Li Li On Oct. 5, 2016, 11:33 p.m., Anne Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52582/ > --- > > (Updated Oct. 5, 2016, 11:33 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya > Tirukkovalur. > > > Bugs: SENTRY-1489 > https://issues.apache.org/jira/browse/SENTRY-1489 > > > Repository: sentry > > > Description > --- > > org.apache.sentry.tests.e2e.dbprovider.TestDbCrossOperations and some other > tests occasionally fail with time out exception. Should make a more flexible > rule for different type of tests. By nature, some tests just take longer time > to finish. > > > Diffs > - > > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > e7cccbfb7955a497f485e3b69e9dc4d9db191473 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbCrossOperations.java > 60812cfa82d318ddfad32e439d3895f49ed21180 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithHiveServer.java > a314c0dde9c60e7dbb51156cea13980bf8600bf1 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 62da0256e8546a25f0ed0a97b8f3c21eec4fecaa > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/RulesForE2ETest.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/SlowE2ETest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52582/diff/ > > > Testing > --- > > > Thanks, > > Anne Yu > >
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
> On Oct. 7, 2016, 5:25 p.m., Anne Yu wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java, > > line 18 > > <https://reviews.apache.org/r/45859/diff/6/?file=1526012#file1526012line18> > > > > Just a minor issue. I've seen across codes, license is on usually the > > top then package definition and imports followed afterwards. > > > > > > https://github.com/apache/hive/blob/master/common/src/java/org/apache/hadoop/hive/common/FileUtils.java good catch! will fix it - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/#review151825 ------- On Oct. 7, 2016, 12:38 a.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45859/ > --- > > (Updated Oct. 7, 2016, 12:38 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, > and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Show role / privileges info in Sentry Service Webpage. Since it is only used > for debug / test currently, this webpage can be seen only when > SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. > > > Diffs > - > > > sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 5ec364c460e74d0a9dae8a28c20042360157b8a0 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java > a42f395270996da345ce49edca909e0438383759 > > Diff: https://reviews.apache.org/r/45859/diff/ > > > Testing > --- > > Already tested in kerberos cluster. When > sentry.service.web.authentication.type is set to KERBEROS, only the > SENTRY_WEB_SECURITY_ALLOW_CONNECT_USERS can see this page. Also this webpage > can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. > > > Thanks, > > Li Li > >
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/ --- (Updated Oct. 7, 2016, 12:38 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Show role / privileges info in Sentry Service Webpage. Since it is only used for debug / test currently, this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Diffs (updated) - sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 5ec364c460e74d0a9dae8a28c20042360157b8a0 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java a42f395270996da345ce49edca909e0438383759 Diff: https://reviews.apache.org/r/45859/diff/ Testing --- Already tested in kerberos cluster. When sentry.service.web.authentication.type is set to KERBEROS, only the SENTRY_WEB_SECURITY_ALLOW_CONNECT_USERS can see this page. Also this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Thanks, Li Li
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
> On Oct. 7, 2016, 12:28 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java, > > line 92 > > <https://reviews.apache.org/r/45859/diff/4/?file=1522419#file1522419line92> > > > > Since you are going to the trouble of disabling cache it makes sense to > > disable it for different browsers and proxies. I see. Will fixed. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/#review151614 ------- On Oct. 7, 2016, 12:26 a.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45859/ > --- > > (Updated Oct. 7, 2016, 12:26 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, > and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Show role / privileges info in Sentry Service Webpage. Since it is only used > for debug / test currently, this webpage can be seen only when > SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. > > > Diffs > - > > > sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 5ec364c460e74d0a9dae8a28c20042360157b8a0 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java > a42f395270996da345ce49edca909e0438383759 > > Diff: https://reviews.apache.org/r/45859/diff/ > > > Testing > --- > > Already tested in kerberos cluster. When > sentry.service.web.authentication.type is set to KERBEROS, only the > SENTRY_WEB_SECURITY_ALLOW_CONNECT_USERS can see this page. Also this webpage > can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. > > > Thanks, > > Li Li > >
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- (Updated Oct. 7, 2016, 12:30 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description --- Add retry logic for non-pool model. Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 842d5cafb06910fcbe6c53002f2101ec5b890a9e sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java abc3f58d21bb774427a34399b6e9f51a37ba51db sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 Diff: https://reviews.apache.org/r/52526/diff/ Testing (updated) --- In non-pool model, for each full retry we will cycle through all available sentry servers. Before each full retry, we will shuffle the server list, and after each full retry, we will have a small random sleep. Thanks, Li Li
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/ --- (Updated Oct. 7, 2016, 12:26 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Show role / privileges info in Sentry Service Webpage. Since it is only used for debug / test currently, this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Diffs (updated) - sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 5ec364c460e74d0a9dae8a28c20042360157b8a0 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java a42f395270996da345ce49edca909e0438383759 Diff: https://reviews.apache.org/r/45859/diff/ Testing --- Already tested in kerberos cluster. When sentry.service.web.authentication.type is set to KERBEROS, only the SENTRY_WEB_SECURITY_ALLOW_CONNECT_USERS can see this page. Also this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Thanks, Li Li
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/ --- (Updated Oct. 4, 2016, 10:23 p.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Changes --- Add flag to enable / disable admin servlet. This webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Repository: sentry Description (updated) --- Show role / privileges info in Sentry Service Webpage. Since it is only used for debug / test currently, this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Diffs (updated) - sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 5ec364c460e74d0a9dae8a28c20042360157b8a0 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java a42f395270996da345ce49edca909e0438383759 Diff: https://reviews.apache.org/r/45859/diff/ Testing (updated) --- Already tested in kerberos cluster. When sentry.service.web.authentication.type is set to KERBEROS, only the SENTRY_WEB_SECURITY_ALLOW_CONNECT_USERS can see this page. Also this webpage can be seen only when SENTRY_WEB_ADMIN_SERVLET_ENABLED is true. Thanks, Li Li
Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/ --- Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description --- Add retry logic for non-pool model. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 4f42a51b1449fe15f856ba252103e66383e175d7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java 3a96d0b124c00efc99cef256c72c25f5c6168007 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 842d5cafb06910fcbe6c53002f2101ec5b890a9e sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java abc3f58d21bb774427a34399b6e9f51a37ba51db sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 5b0e12bbf12510d8d424aa2b7f51076a913234c5 Diff: https://reviews.apache.org/r/52526/diff/ Testing --- Thanks, Li Li
Re: Review Request 52150: SENTRY-1478: Disable fencing in Sentry store for Active/Active
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52150/ --- (Updated Sept. 30, 2016, 12:18 a.m.) Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya Tirukkovalur. Repository: sentry Description (updated) --- This is for design V2.1. Keep the Fencer and Activator classes and disable fencing that brought by SENTRY-1317 and SENTRY-1399, including some tests cases (e.g. TestActivator). Diffs (updated) - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 6f8239dcf2ae23962efe6bb0edf12d3c14e1a038 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java 39e55c3d9cb17ed498aaad98f1c490401c12030b sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e2b92f919c8d4f2bb347ee70c5c623559d57002a sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java a261d8db5b0371514c55771ecb9c2e93a6976437 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 894fcc966b511ccf309599fd10960f9a11ae8e96 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java 353d461d84eb8438506e4bb209828feb98183fa7 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 74977193760dd1958bdafaa3d6dac1ba27d81f32 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java c52197f1ca514604a01ce4f4d9d7d35de30994a0 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java fd10a7a23bcc21786323a27f36aba918f77e819c sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java 42b67ba4078d998a069aa6600b1ab24011b7dd26 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 06342063965a0e6b295fbdbaf2dfc517a2045d60 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java 98f18314d387a831707ba5f06836ae41926878db sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java e401859ab492c1d2a7634361fa301b3350661929 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestFailover.java 57d579cc6179319e3525424d652d224323e21457 Diff: https://reviews.apache.org/r/52150/diff/ Testing --- Thanks, Li Li
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150295 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 214) <https://reviews.apache.org/r/52138/#comment218200> how about adding 'TODO: expose time used for full update in the metrics' ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 257) <https://reviews.apache.org/r/52138/#comment218199> shall we update currentEventID with eventIDBefore(or eventIDAfter).getEventId(), so that client will process notification events from eventIDBefore + 1 ? - Li Li On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Sept. 23, 2016, 6:15 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150190 --- Ship it! Ship It! - Li Li On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Sept. 23, 2016, 6:15 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150116 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 217) <https://reviews.apache.org/r/52138/#comment217984> require -> requires sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 229) <https://reviews.apache.org/r/52138/#comment217986> From the comment, seems you want to update eventIDBefore before fetchFullUpdate() in the while loop sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 252) <https://reviews.apache.org/r/52138/#comment217979> I think we don't need 'currentRetries == maxRetriesForHMSSnapshot' here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 257) <https://reviews.apache.org/r/52138/#comment217987> why set fullUpdateComplete to false here? Seems fullUpdateComplete is always false now. - Li Li On Sept. 23, 2016, 12:50 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Sept. 23, 2016, 12:50 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 52150: SENTRY-1478: Disable fencing in Sentry store for Active/Active
> On Sept. 22, 2016, 7:56 p.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java, > > line 97 > > <https://reviews.apache.org/r/52150/diff/1/?file=1507892#file1507892line97> > > > > I think we may still want to keep the activator code, and just disable > > fencing in sentry store. So we can have a backup as active/passive model. you are right. Even for active / active mode, we still have only 1 deamon that fetch the snapshot from hms and write to backend db, so we'd Besides, we can keep the activator code. Besides, we can use it for internal testing / debugging that resolve Anne the concern. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52150/#review150040 ----------- On Sept. 22, 2016, 5:27 a.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52150/ > --- > > (Updated Sept. 22, 2016, 5:27 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Keep the Fencer and Activator classes and disable fencing that brought by > SENTRY-1317 and SENTRY-1399, including some tests cases (e.g. TestActivator). > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 6f8239dcf2ae23962efe6bb0edf12d3c14e1a038 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > 39e55c3d9cb17ed498aaad98f1c490401c12030b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > e2b92f919c8d4f2bb347ee70c5c623559d57002a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 353d461d84eb8438506e4bb209828feb98183fa7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 74977193760dd1958bdafaa3d6dac1ba27d81f32 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java > c52197f1ca514604a01ce4f4d9d7d35de30994a0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java > fd10a7a23bcc21786323a27f36aba918f77e819c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java > 42b67ba4078d998a069aa6600b1ab24011b7dd26 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 06342063965a0e6b295fbdbaf2dfc517a2045d60 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > 98f18314d387a831707ba5f06836ae41926878db > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java > e401859ab492c1d2a7634361fa301b3350661929 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestActivator.java > 60cfc735832433fb4dbeae1c2d617dd713fc3f3e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestFailover.java > 57d579cc6179319e3525424d652d224323e21457 > > Diff: https://reviews.apache.org/r/52150/diff/ > > > Testing > --- > > > Thanks, > > Li Li > >
Re: Review Request 52150: SENTRY-1478: Disable fencing in Sentry store for Active/Active
> On Sept. 22, 2016, 6:34 p.m., Sravya Tirukkovalur wrote: > > Thanks for the patch Li Li! This change seems to be from design V2.1. I > > think it makes sense to mention that in the jira and also wait for a few > > days for the community to review the new design before moving ahead. What > > do you think? thanks, Sravya! I agree. - Li --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52150/#review150043 --- On Sept. 22, 2016, 5:27 a.m., Li Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52150/ > --- > > (Updated Sept. 22, 2016, 5:27 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > Keep the Fencer and Activator classes and disable fencing that brought by > SENTRY-1317 and SENTRY-1399, including some tests cases (e.g. TestActivator). > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 6f8239dcf2ae23962efe6bb0edf12d3c14e1a038 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > 39e55c3d9cb17ed498aaad98f1c490401c12030b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > e2b92f919c8d4f2bb347ee70c5c623559d57002a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > 353d461d84eb8438506e4bb209828feb98183fa7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 74977193760dd1958bdafaa3d6dac1ba27d81f32 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java > c52197f1ca514604a01ce4f4d9d7d35de30994a0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java > fd10a7a23bcc21786323a27f36aba918f77e819c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java > 42b67ba4078d998a069aa6600b1ab24011b7dd26 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 06342063965a0e6b295fbdbaf2dfc517a2045d60 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > 98f18314d387a831707ba5f06836ae41926878db > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java > e401859ab492c1d2a7634361fa301b3350661929 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestActivator.java > 60cfc735832433fb4dbeae1c2d617dd713fc3f3e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestFailover.java > 57d579cc6179319e3525424d652d224323e21457 > > Diff: https://reviews.apache.org/r/52150/diff/ > > > Testing > --- > > > Thanks, > > Li Li > >
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150070 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 206) <https://reviews.apache.org/r/52138/#comment217880> return if client == null, and throw / log the exception sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 216) <https://reviews.apache.org/r/52138/#comment217886> could we change the currentEventID to a different name, since we have a var with the same name in line 64 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 219) <https://reviews.apache.org/r/52138/#comment217882> I think we should give up if fetchFullUpdate() fails, since FullUpdateInitializer also has retry, and retry here is just to fetch the latest snapshot. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 220) <https://reviews.apache.org/r/52138/#comment217891> Shall we update currentEventID before retry? Shall we use !equals instead of != ? - Li Li On Sept. 22, 2016, 8:39 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > --- > > (Updated Sept. 22, 2016, 8:39 p.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > --- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > --- > > > Thanks, > > Hao Hao > >
Re: Review Request 50578: Sentry-1411: The sentry client should retry RPCs if it gets a SentryStandbyException
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50578/#review149898 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 278) <https://reviews.apache.org/r/50578/#comment217665> endpointIdx -> i ? also in line282 Also consider the situation when exec[i] == null - Li Li On Aug. 2, 2016, 9:55 p.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50578/ > --- > > (Updated Aug. 2, 2016, 9:55 p.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 > b2df699b962def9ef2dbf60fe8f601f9aaeb03f5 > > 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 > 56d774b883914db438101f47fb94518c94d5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java > afea78a0bea754f950cc7e3033e1b1ebfdcb35fc > > 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 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/ --- (Updated Sept. 19, 2016, 11:29 p.m.) Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Show role / privileges info in Sentry Service Webpage Diffs (updated) - sentry-service/sentry-service-server/pom.xml be165b6d43fa2f902749458634ab64bdc9b1d044 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java a42f395270996da345ce49edca909e0438383759 sentry-service/sentry-service-server/src/main/resources/realm.properties PRE-CREATION sentry-service/sentry-service-server/src/main/webapp/SentryService.html 9eb5f0eb4743c1215caf99a90bc89e810b21db87 Diff: https://reviews.apache.org/r/45859/diff/ Testing --- Thanks, Li Li
Re: Review Request 45859: SENTRY-1120: Show role / privileges info in Sentry Service Webpage
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45859/ --- (Updated April 23, 2016, 1:06 a.m.) Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur. Repository: sentry Description --- Show role / privileges info in Sentry Service Webpage Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/AdminServlet.java PRE-CREATION sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f Diff: https://reviews.apache.org/r/45859/diff/ Testing --- Thanks, Li Li
Re: need update sentry wiki permission
Thanks, Hao! I just updated the wiki. Best, Li On Wed, Mar 30, 2016 at 3:00 PM, Hao Hao <hao@cloudera.com> wrote: > Hi Li, > > Just grant the edit permission to you. Thanks! > > Best, > Hao > > On Wed, Mar 30, 2016 at 2:23 PM, Li Li <l...@cloudera.com> wrote: > > > Hey, guys, > > > > I am working on SENTRY-1166, which is to update default value for > > sentry.hive.server in Sentry wiki page. However,I don't have the > permission > > to update the wiki page > > < > > > https://cwiki.apache.org/confluence/display/SENTRY/Sentry+Service+Configuration > > >. > > I wonder if anyone can help to grant me the permission. > > > > Thanks, > > Li > > >
