date:20180221

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

2018-02-21 Thread Xinran Tinney



> On Feb. 21, 2018, 6:16 p.m., Sergio Pena wrote:
> > Why is the TestHDFSIntegration.java deleted? I see it has other tests in 
> > that class.

I am not sure, if we still need it, I will resubmit the patch.


- Xinran


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


On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65715/
> ---
> 
> (Updated Feb. 20, 2018, 8:37 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it 
> or remove if it is no longer applicable.
> The TestHDFSIntegration test is disabled as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  8852cbc0 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  92c0693e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  96a2f901 
> 
> 
> Diff: https://reviews.apache.org/r/65715/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean install all success
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

2018-02-21 Thread Xinran Tinney



> On Feb. 21, 2018, 7:11 p.m., kalyan kumar kalvagadda wrote:
> > Xinren,
> > 
> > 
> > Can you hold on on this patch. I had a patch before which extends the HA 
> > functionality to sentry tests. That patch was not commiited because of test 
> > failures. 
> > Instead of deleteting it I would prefer fixing it to test HA.

Sure, if we need to keep it, I will modify.


- Xinran


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


On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65715/
> ---
> 
> (Updated Feb. 20, 2018, 8:37 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it 
> or remove if it is no longer applicable.
> The TestHDFSIntegration test is disabled as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  8852cbc0 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  92c0693e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  96a2f901 
> 
> 
> Diff: https://reviews.apache.org/r/65715/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean install all success
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board



> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 137-140 (patched)
> > 
> >
> > What if oldDbName and newDbname are nulls but oldTablename and 
> > newTableName are not null and different? Should we consider it as a table 
> > rename?
> > 
> > Would the following conditions be simpler?
> > 
> > if (oldTableName != null && newTableName != null) {
> >if (oldTableName.equalsIgnoreCase(newTableName) {
> > return true;
> >   }
> > }
> > 
> > if (oldDbName != null && newDbname != null) {
> >   if (oldDbName.equalsIgnoreCase(newDbname) {
> > return true;
> >   }
> > }
> 
> Na Li wrote:
> I am following the logic in Notification.processAlterTable. We need to 
> keep the same logic in this function and Notification.processAlterTable
> 
> Sergio Pena wrote:
> If we need to keep the same logic, would make sense to have a shared 
> method that checks if a table is renamed? Also, the Notification method 
> checks for path changes as well (not just rename), so does it mean we sync 
> with these changes as well? Notice that locations are useful for HDFS sync 
> only not grant/revoke privileges.
> 
> Na Li wrote:
> I cannot. The input here is of type AlterTableEvent. The input in 
> Notification.processAlterTable is NotificationEvent. It is too much trouble 
> to use the same function for such simple logic

seems to me, even for other alter table cases, it is impossible for one the 
these {old db name, new db name, old table name, new table name} to be null. 
what does it mean when one of them is null?


- Na


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


On Feb. 22, 2018, 1:28 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 22, 2018, 1:28 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  edea5b6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/3/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board



> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 137-140 (patched)
> > 
> >
> > What if oldDbName and newDbname are nulls but oldTablename and 
> > newTableName are not null and different? Should we consider it as a table 
> > rename?
> > 
> > Would the following conditions be simpler?
> > 
> > if (oldTableName != null && newTableName != null) {
> >if (oldTableName.equalsIgnoreCase(newTableName) {
> > return true;
> >   }
> > }
> > 
> > if (oldDbName != null && newDbname != null) {
> >   if (oldDbName.equalsIgnoreCase(newDbname) {
> > return true;
> >   }
> > }
> 
> Na Li wrote:
> I am following the logic in Notification.processAlterTable. We need to 
> keep the same logic in this function and Notification.processAlterTable
> 
> Sergio Pena wrote:
> If we need to keep the same logic, would make sense to have a shared 
> method that checks if a table is renamed? Also, the Notification method 
> checks for path changes as well (not just rename), so does it mean we sync 
> with these changes as well? Notice that locations are useful for HDFS sync 
> only not grant/revoke privileges.

I cannot. The input here is of type AlterTableEvent. The input in 
Notification.processAlterTable is NotificationEvent. It is too much trouble to 
use the same function for such simple logic


- Na


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


On Feb. 22, 2018, 1:28 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 22, 2018, 1:28 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  edea5b6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/3/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board


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

(Updated Feb. 22, 2018, 1:28 a.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
Sergio Pena.


Repository: sentry


Description
---

wait for HMS sync at alter table, which including table rename and changing 
table columns


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 24d7763 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 edea5b6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4cd00e6 


Diff: https://reviews.apache.org/r/65704/diff/3/

Changes: https://reviews.apache.org/r/65704/diff/2-3/


Testing
---

unit tests succeeded


Thanks,

Na Li

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-02-21 Thread Alexander Kolbasov


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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 106 (patched)


I would consider changing default here from 250ms to 125 ms. With the 
current sleep the first delay is closer to 400-500 ms which is a bit too much 
for initial sleep
with 125 ms default we get initial sleep close to 200 ms while not 
increasing retries a lot.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3432 (patched)


30 sec is too much for this test - it delays the whole test for 30 sec. Can 
you test it with the whole test taking ~2 seconds?


- Alexander Kolbasov


On Feb. 12, 2018, 11:50 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 12, 2018, 11:50 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1904
> https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TransactionManager uses exponential backoff strategy for transaction 
> retries. This may cause some transactions to be delayed by a very long time. 
> We should also have a constraint on the max time for a transaction so that we 
> do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction 
> could be be active.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
>  f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/8/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Sergio Pena via Review Board



> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 137-140 (patched)
> > 
> >
> > What if oldDbName and newDbname are nulls but oldTablename and 
> > newTableName are not null and different? Should we consider it as a table 
> > rename?
> > 
> > Would the following conditions be simpler?
> > 
> > if (oldTableName != null && newTableName != null) {
> >if (oldTableName.equalsIgnoreCase(newTableName) {
> > return true;
> >   }
> > }
> > 
> > if (oldDbName != null && newDbname != null) {
> >   if (oldDbName.equalsIgnoreCase(newDbname) {
> > return true;
> >   }
> > }
> 
> Na Li wrote:
> I am following the logic in Notification.processAlterTable. We need to 
> keep the same logic in this function and Notification.processAlterTable

If we need to keep the same logic, would make sense to have a shared method 
that checks if a table is renamed? Also, the Notification method checks for 
path changes as well (not just rename), so does it mean we sync with these 
changes as well? Notice that locations are useful for HDFS sync only not 
grant/revoke privileges.


- Sergio


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


On Feb. 21, 2018, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 21, 2018, 4:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread kalyan kumar kalvagadda via Review Board

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

I'm not sure if sentry has a test that performs a table rename and immediatly 
peforms an hive operation that uses the new table name. I did a quick search 
and did not find any.
If sentry doesn't have them we need to add that e2e test.

Somthing like this
**Case-1:**
1. grant all on db1.tb1 to a role1.
2. alter table d1.tb1 rename to d1.tb_new; //Changing the name of the table.
3. perform "select * from d1.tb_new" as role1 and make sure that that 
authorization is granted.

**Case-2:**
1. grant all on db1.tb1 to a role1.
2. alter table d1.tb1 rename to d2.tb1; //Moving table from another database to 
another. 
3. perform "select * from d2.tb1" as role1 and make sure that that 
authorization is granted.

- kalyan kumar kalvagadda

On Feb. 21, 2018, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 21, 2018, 4:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-21 Thread Sergio Pena via Review Board


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


Ship it!




Ship It!

- Sergio Pena


On Feb. 21, 2018, 6:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65533/
> ---
> 
> (Updated Feb. 21, 2018, 6:45 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2115
> https://issues.apache.org/jira/browse/SENTRY-2115
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> **Scenario-1:** When HDFS sync is disabled, and sentry is started for the 
> first time.
> **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
> less than last event-d processed by sentry
> **Scenario-3:** When HDFS sync is disabled, and first event-id in the 
> subsequent pull is not greater than the last event-id processed by sentry by 
> 1.
> **New Behavior:** Full snapshots need not be taken in all
> When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> 
> **Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
> and then HDFS sync is enabled and sentry service is restarted to get it to 
> effect.
> **New Behavior:** When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> To handle scenario explained in Scenario-4, sentry should reset the mapping 
> information when ever HDFS sync is disabled. That way it can learn from 
> scratch when the feature is enabled back. There is no value is holding stale 
> data even when we know it will have issues when the feature is enabled back.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  eae7861728f2bc11b4c1b44aa3b61b881a87740b 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  cf764eda1a006ce96f301e3ecb87749e05ba4a09 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  e7558370025c6acd83492615be093f2bd16a202b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  79030780c35e5bda432e3ec3f01328e627cb50a6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6672730773c74b9840247d1f4d5f7bdfe4 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  e64f5cd687bf59133d6475c912ebdd7930601151 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  91397061e78a6524410d35643dd3a33be8353ecc 
> 
> 
> Diff: https://reviews.apache.org/r/65533/diff/4/
> 
> 
> Testing
> ---
> 
> Made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-02-21 Thread Sergio Pena via Review Board


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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 262 (patched)


If the maxRetryTimeIntervalMillis is lower than 1/3 of the maxRetryTime, 
then we will start seeing the same sleepTime is used in >50% of all retries 
until the end of the maxDurationTime.

Should we need introduce the randomness after the final sleepTime 
calculation?

Attempt#1   420
Attempt#2   704
Attempt#3  1281
Attempt#4  2069
Attempt#5  3498
Attempt#6  6360
Attempt#7  7500
Attempt#8  7500
Attempt#9  7500
Attempt#10 7500
Attempt#11 7500
Attempt#12 7500
Attempt#13 7500
Attempt#14 7500
Attempt#15 7500
Attempt#16 7369



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 265 (patched)


By moving this sleep to the end, the initial sleep time which used to be 
250ms is not used but the next time.

The old code used to sleep initially for 250ms.
The new code is incrementing the sleep time that could be up to 500ms for 
the initial sleep.

Is this the new expected behavior? Shouldn't we move the sleep() call 
before making the new sleepTime calculation?


- Sergio Pena


On Feb. 12, 2018, 11:50 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 12, 2018, 11:50 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1904
> https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TransactionManager uses exponential backoff strategy for transaction 
> retries. This may cause some transactions to be delayed by a very long time. 
> We should also have a constraint on the max time for a transaction so that we 
> do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction 
> could be be active.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
>  f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/8/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board



> On Feb. 21, 2018, 7:01 p.m., kalyan kumar kalvagadda wrote:
> > I'm not sure if sentry has a test that performs a table rename and 
> > immediatly peforms an hive operation that uses the new table name. I did a 
> > quick search and did not find any.
> > If sentry doesn't have them we need to add that e2e test.
> > 
> > Somthing like this
> > **Case-1:**
> > 1. grant all on db1.tb1 to a role1.
> > 2. alter table d1.tb1 rename to d1.tb_new; //Changing the name of the table.
> > 3. perform "select * from d1.tb_new" as role1 and make sure that that 
> > authorization is granted.
> > 
> > **Case-2:**
> > 1. grant all on db1.tb1 to a role1.
> > 2. alter table d1.tb1 rename to d2.tb1; //Moving table from another 
> > database to another. 
> > 3. perform "select * from d2.tb1" as role1 and make sure that that 
> > authorization is granted.

we do have such test TestDbPrivilegeCleanupOnDrop.testRenameTables. Since it 
renames four tables at once, the delay may be long enough for it to always 
works. I can add a test case only rename single table and access privilege of 
old and new tables right after.


- Na


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


On Feb. 21, 2018, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 21, 2018, 4:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board



> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 137-140 (patched)
> > 
> >
> > What if oldDbName and newDbname are nulls but oldTablename and 
> > newTableName are not null and different? Should we consider it as a table 
> > rename?
> > 
> > Would the following conditions be simpler?
> > 
> > if (oldTableName != null && newTableName != null) {
> >if (oldTableName.equalsIgnoreCase(newTableName) {
> > return true;
> >   }
> > }
> > 
> > if (oldDbName != null && newDbname != null) {
> >   if (oldDbName.equalsIgnoreCase(newDbname) {
> > return true;
> >   }
> > }

I am following the logic in Notification.processAlterTable. We need to keep the 
same logic in this function and Notification.processAlterTable


> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 141-147 (patched)
> > 
> >
> > Why do we need an error logging here? The Sentry server will log this 
> > error again. I think we just need to return true or false whether the 
> > rename happened or not regardless if the event had enough information, 
> > don't we? This log will be in the HMS side nor Sentry, so it would be 
> > verbose for customres.

I will change it to debug level log. It would help debuging if we suspect table 
rename was not synced.


> On Feb. 21, 2018, 5:13 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
> > Lines 670 (patched)
> > 
> >
> > This comment is out of place, isn't it?

yes. will move to right place


- Na


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


On Feb. 21, 2018, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 21, 2018, 4:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

2018-02-21 Thread kalyan kumar kalvagadda via Review Board


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



Xinren,


Can you hold on on this patch. I had a patch before which extends the HA 
functionality to sentry tests. That patch was not commiited because of test 
failures. 
Instead of deleteting it I would prefer fixing it to test HA.

- kalyan kumar kalvagadda


On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65715/
> ---
> 
> (Updated Feb. 20, 2018, 8:37 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it 
> or remove if it is no longer applicable.
> The TestHDFSIntegration test is disabled as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  8852cbc0 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  92c0693e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  96a2f901 
> 
> 
> Diff: https://reviews.apache.org/r/65715/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean install all success
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 65641: SENTRY-853 Handle show grant on failure correctly

2018-02-21 Thread Sergio Pena via Review Board


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


Ship it!




Got it, Thanks Steve.

- Sergio Pena


On Feb. 13, 2018, 10:35 p.m., Steve Moist wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65641/
> ---
> 
> (Updated Feb. 13, 2018, 10:35 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Improved the error message for when a show command is run instead of a 
> generic message that is used improperly.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java
>  38d1f468 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
>  1e520c0b 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  14a96191 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java
>  c75f57d6 
> 
> 
> Diff: https://reviews.apache.org/r/65641/diff/1/
> 
> 
> Testing
> ---
> 
> Setup cluster and ran commands through beeline:
> 0: jdbc:hive2://server> show grant user usercomedy;
> Error: Error while compiling statement: FAILED: SemanticException Sentry does 
> not allow privileges to be shown for: USER (state=42000,code=4)
> 0: jdbc:hive2://server> show grant on table movies;
> Error: Error while compiling statement: FAILED: SemanticException Sentry does 
> not allow privileges to be shown for: USER (state=42000,code=4)
> 0: jdbc:hive2://server> show grant on database moviesdb;
> Error: Error while compiling statement: FAILED: SemanticException Sentry does 
> not allow privileges to be shown for: USER (state=42000,code=4)
> 0: jdbc:hive2://server> show grant group comedy_group;
> Error: Error while compiling statement: FAILED: SemanticException Sentry does 
> not allow privileges to be shown for: GROUP (state=42000,code=4)
> 0: jdbc:hive2://server> show role grant role comedyrole;
> Error: Error while compiling statement: FAILED: SemanticException Sentry does 
> not allow privileges to be shown for: ROLE (state=42000,code=4)
> 
> 
> Thanks,
> 
> Steve Moist
> 
>

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-21 Thread kalyan kumar kalvagadda via Review Board


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

(Updated Feb. 21, 2018, 6:45 p.m.)


Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.


Changes
---

Lina,

It was my issue. I did not updated the patch properly.


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


Repository: sentry


Description
---

**Scenario-1:** When HDFS sync is disabled, and sentry is started for the first 
time.
**Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
less than last event-d processed by sentry
**Scenario-3:** When HDFS sync is disabled, and first event-id in the 
subsequent pull is not greater than the last event-id processed by sentry by 1.
**New Behavior:** Full snapshots need not be taken in all
When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.

**Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
and then HDFS sync is enabled and sentry service is restarted to get it to 
effect.
**New Behavior:** When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.
To handle scenario explained in Scenario-4, sentry should reset the mapping 
information when ever HDFS sync is disabled. That way it can learn from scratch 
when the feature is enabled back. There is no value is holding stale data even 
when we know it will have issues when the feature is enabled back.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 eae7861728f2bc11b4c1b44aa3b61b881a87740b 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 cf764eda1a006ce96f301e3ecb87749e05ba4a09 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 2f2b98412e7dfdcc847ffe7975a70f452554e747 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 e7558370025c6acd83492615be093f2bd16a202b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 79030780c35e5bda432e3ec3f01328e627cb50a6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4cd00e6672730773c74b9840247d1f4d5f7bdfe4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 PRE-CREATION 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 e64f5cd687bf59133d6475c912ebdd7930601151 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
 91397061e78a6524410d35643dd3a33be8353ecc 


Diff: https://reviews.apache.org/r/65533/diff/4/

Changes: https://reviews.apache.org/r/65533/diff/3-4/


Testing
---

Made sure that all the tests passed.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

2018-02-21 Thread Sergio Pena via Review Board


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


Ship it!




Thanks Xinran, then I think this change is correct as the SentryMain would 
haven't run if the JAR location was incorrect.

- Sergio Pena


On Jan. 10, 2018, 9:38 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> ---
> 
> (Updated Jan. 10, 2018, 9:38 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TheSentryMain class currently works by mapping the command name to a Java 
> class that is then dynamically loaded:
> String commandName = commandLine.getOptionValue(COMMAND);
> String commandClazz = COMMANDS.get(commandName);
> Object command;
> try {
>   command = Class.forName(commandClazz.trim()).newInstance();
> } catch (Exception e) {
>   String msg = "Could not create instance of " + commandClazz + " for 
> command " + commandName;
>   throw new IllegalStateException(msg, e);
> }
> if (!(command instanceof Command)) {
>   String msg = "Command " + command.getClass().getName() + " is not an 
> instance of "
>   + Command.class.getName();
>   throw new IllegalStateException(msg);
> }
> ((Command)command).run(commandLine.getArgs());
>   }
> This ia too complicated and causes subtle problems at runtime. Instead it 
> should just create a new instance of appropriate class and call it directly.
> 
> 
> Diffs
> -
> 
>   bin/sentry 54e545aa 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  3a981b2a 
>   sentry-tools/pom.xml 45cfdb56 
>   sentry-tools/src/main/java/org/apache/sentry/SentryMain.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64259/diff/5/
> 
> 
> Testing
> ---
> 
> mvn clean install, on cloudcat and all SUCCESS
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

2018-02-21 Thread Sergio Pena via Review Board


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



Why is the TestHDFSIntegration.java deleted? I see it has other tests in that 
class.

- Sergio Pena


On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65715/
> ---
> 
> (Updated Feb. 20, 2018, 8:37 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it 
> or remove if it is no longer applicable.
> The TestHDFSIntegration test is disabled as well.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  8852cbc0 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  92c0693e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java
>  96a2f901 
> 
> 
> Diff: https://reviews.apache.org/r/65715/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean install all success
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Sergio Pena via Review Board


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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 129 (patched)


Can you add Javadoc in this method?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 135 (patched)


isn't tableEvent.getNewTable() null for any other ALTER event that does not 
involve renaming?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 137-140 (patched)


What if oldDbName and newDbname are nulls but oldTablename and newTableName 
are not null and different? Should we consider it as a table rename?

Would the following conditions be simpler?

if (oldTableName != null && newTableName != null) {
   if (oldTableName.equalsIgnoreCase(newTableName) {
return true;
  }
}

if (oldDbName != null && newDbname != null) {
  if (oldDbName.equalsIgnoreCase(newDbname) {
return true;
  }
}



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 141-147 (patched)


Why do we need an error logging here? The Sentry server will log this error 
again. I think we just need to return true or false whether the rename happened 
or not regardless if the event had enough information, don't we? This log will 
be in the HMS side nor Sentry, so it would be verbose for customres.



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
Lines 670 (patched)


This comment is out of place, isn't it?


- Sergio Pena


On Feb. 21, 2018, 4:52 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 21, 2018, 4:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/2/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

FINAL REMINDER: CFP for Apache EU Roadshow Closes 25th February

2018-02-21 Thread Sharan F


Hello Apache Supporters and Enthusiasts

This is your FINAL reminder that the Call for Papers (CFP) for the 
Apache EU Roadshow is closing soon. Our Apache EU Roadshow will focus on 
Cloud, IoT, Apache Tomcat, Apache Http and will run from 13-14 June 2018 
in Berlin.
Note that the CFP deadline has been extended to *25*^*th* *February *and 
it will be your final opportunity to submit a talk for thisevent.


Please make your submissions at http://apachecon.com/euroadshow18/

Also note that early bird ticket registrations to attend FOSS Backstage 
including the Apache EU Roadshow, have also been extended and will be 
available until 23^rd February. Please register at 
https://foss-backstage.de/tickets


We look forward to seeing you in Berlin!

Thanks
Sharan Foga, VP Apache Community Development

PLEASE NOTE: You are receiving this message because you are subscribed 
to a user@ or dev@ list of one or more Apache Software Foundation projects.

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

2018-02-21 Thread Na Li via Review Board


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

(Updated Feb. 21, 2018, 4:52 p.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
Sergio Pena.


Repository: sentry


Description
---

wait for HMS sync at alter table, which including table rename and changing 
table columns


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 24d7763 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 4cd00e6 


Diff: https://reviews.apache.org/r/65704/diff/2/


Testing (updated)
---

unit tests succeeded


Thanks,

Na Li

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-21 Thread Na Li via Review Board



> On Feb. 20, 2018, 8:28 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
> > Line 336 (original), 354 (patched)
> > 
> >
> > why do you comment out this line?

This is not fixed. Did you upload the latest patch?


> On Feb. 20, 2018, 8:28 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
> > Line 999 (original), 1161 (patched)
> > 
> >
> > if you don't need those lines, can you just remove them?

This is not fixed. Did you upload the latest patch?


- Na


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


On Feb. 15, 2018, 9 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65533/
> ---
> 
> (Updated Feb. 15, 2018, 9 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2115
> https://issues.apache.org/jira/browse/SENTRY-2115
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> **Scenario-1:** When HDFS sync is disabled, and sentry is started for the 
> first time.
> **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
> less than last event-d processed by sentry
> **Scenario-3:** When HDFS sync is disabled, and first event-id in the 
> subsequent pull is not greater than the last event-id processed by sentry by 
> 1.
> **New Behavior:** Full snapshots need not be taken in all
> When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> 
> **Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
> and then HDFS sync is enabled and sentry service is restarted to get it to 
> effect.
> **New Behavior:** When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> To handle scenario explained in Scenario-4, sentry should reset the mapping 
> information when ever HDFS sync is disabled. That way it can learn from 
> scratch when the feature is enabled back. There is no value is holding stale 
> data even when we know it will have issues when the feature is enabled back.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  eae7861728f2bc11b4c1b44aa3b61b881a87740b 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  cf764eda1a006ce96f301e3ecb87749e05ba4a09 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  e7558370025c6acd83492615be093f2bd16a202b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  79030780c35e5bda432e3ec3f01328e627cb50a6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6672730773c74b9840247d1f4d5f7bdfe4 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  e64f5cd687bf59133d6475c912ebdd7930601151 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  91397061e78a6524410d35643dd3a33be8353ecc 
> 
> 
> Diff: https://reviews.apache.org/r/65533/diff/3/
> 
> 
> Testing
> ---
> 
> Made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

Re: Review Request 65641: SENTRY-853 Handle show grant on failure correctly

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

FINAL REMINDER: CFP for Apache EU Roadshow Closes 25th February

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

21 matches

Site Navigation

Mail list logo

Footer information