Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated May 18, 2018, 8:03 p.m.)


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


Repository: sentry


Description (updated)
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename

This fix only handles notification event processing when there is no full 
snapshot. The changes related to update to full snapshot is tracked in 
"SENTRY-2239 FullUpdateModifier does not handle alter table rename correctly"


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 cafe2b5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


Diff: https://reviews.apache.org/r/65768/diff/5/

Changes: https://reviews.apache.org/r/65768/diff/4-5/


Testing
---

unit test succeeded


Thanks,

Na Li

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[kalyan kumar kalvagadda via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
Lines 364-379 (original), 364-373 (patched)


I don't quite understand this logic.

alter table could do couple of things
1. alter the table name (alter table db1.tb1 rename to db1.tb2)
2. move the table from one database to another one (alter table db1.tb2 
rename to db2.tb2;)
3. both (alter table db1.tb2 rename to db2.tb3)

From what I understand, difference in prevDbName and newDbName means that 
table is moved from one database to another.


- kalyan kumar kalvagadda


On April 10, 2018, 7:22 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated April 10, 2018, 7:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  83c0fc4 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/4/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

> On March 14, 2018, 6:33 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 363 (original), 363 (patched)
> > 
> >
> > if changing the database is not supported hive should throw an error, 
> > if hive allows that command sentry should be handling it.
> > 1. update the path information
> > 2. change the permission
> 
> Na Li wrote:
> The function that processes the event is alterTable(), not 
> alterDatabase(). If user wants to change the name of a database, the event 
> handler should call alterDatabase(), not alterTable(). The current processing 
> assumes that when user calls alter table and the database name is changed, 
> then all tables in original database should change their database name. I 
> don't think this assumption is valid. It is very dangerous to do so. 
> 
> altertable should only be used to change table, not all tables in a 
> database.
> 
> NotificationProcessor.processAlterTable does not do this: "when user 
> calls alter table and the database name is changed, then all tables in 
> original database should change their database name."
> 
> kalyan kumar kalvagadda wrote:
> You are right. This part of code seems to be incorrect. you may want to 
> fix it as well in this commit as it is related.
> 
> kalyan kumar kalvagadda wrote:
> Lina, what is the latest on this? you you planning to fix the code in 
> FullUpdateModifier.java?

I have removed the code to change all tables in db in alter table processing.


- Na


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


On April 10, 2018, 7:22 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated April 10, 2018, 7:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  83c0fc4 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/4/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated April 10, 2018, 7:22 p.m.)


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


Repository: sentry


Description
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 83c0fc4 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 c30d982 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 b410027 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 c6be80d 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


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

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


Testing
---

unit test succeeded


Thanks,

Na Li

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[kalyan kumar kalvagadda via Review Board]

> On March 14, 2018, 6:33 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 363 (original), 363 (patched)
> > 
> >
> > if changing the database is not supported hive should throw an error, 
> > if hive allows that command sentry should be handling it.
> > 1. update the path information
> > 2. change the permission
> 
> Na Li wrote:
> The function that processes the event is alterTable(), not 
> alterDatabase(). If user wants to change the name of a database, the event 
> handler should call alterDatabase(), not alterTable(). The current processing 
> assumes that when user calls alter table and the database name is changed, 
> then all tables in original database should change their database name. I 
> don't think this assumption is valid. It is very dangerous to do so. 
> 
> altertable should only be used to change table, not all tables in a 
> database.
> 
> NotificationProcessor.processAlterTable does not do this: "when user 
> calls alter table and the database name is changed, then all tables in 
> original database should change their database name."
> 
> kalyan kumar kalvagadda wrote:
> You are right. This part of code seems to be incorrect. you may want to 
> fix it as well in this commit as it is related.

Lina, what is the latest on this? you you planning to fix the code in 
FullUpdateModifier.java?


- kalyan kumar


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


On March 7, 2018, 10:39 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated March 7, 2018, 10:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/3/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[kalyan kumar kalvagadda via Review Board]

> On March 14, 2018, 6:33 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 363 (original), 363 (patched)
> > 
> >
> > if changing the database is not supported hive should throw an error, 
> > if hive allows that command sentry should be handling it.
> > 1. update the path information
> > 2. change the permission
> 
> Na Li wrote:
> The function that processes the event is alterTable(), not 
> alterDatabase(). If user wants to change the name of a database, the event 
> handler should call alterDatabase(), not alterTable(). The current processing 
> assumes that when user calls alter table and the database name is changed, 
> then all tables in original database should change their database name. I 
> don't think this assumption is valid. It is very dangerous to do so. 
> 
> altertable should only be used to change table, not all tables in a 
> database.
> 
> NotificationProcessor.processAlterTable does not do this: "when user 
> calls alter table and the database name is changed, then all tables in 
> original database should change their database name."

You are right. This part of code seems to be incorrect. you may want to fix it 
as well in this commit as it is related.


- kalyan kumar


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


On March 7, 2018, 10:39 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated March 7, 2018, 10:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/3/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

> On March 14, 2018, 6:33 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 363 (original), 363 (patched)
> > 
> >
> > if changing the database is not supported hive should throw an error, 
> > if hive allows that command sentry should be handling it.
> > 1. update the path information
> > 2. change the permission

The function that processes the event is alterTable(), not alterDatabase(). If 
user wants to change the name of a database, the event handler should call 
alterDatabase(), not alterTable(). The current processing assumes that when 
user calls alter table and the database name is changed, then all tables in 
original database should change their database name. I don't think this 
assumption is valid. It is very dangerous to do so. 

altertable should only be used to change table, not all tables in a database.

NotificationProcessor.processAlterTable does not do this: "when user calls 
alter table and the database name is changed, then all tables in original 
database should change their database name."


- Na


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


On March 7, 2018, 10:39 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated March 7, 2018, 10:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/3/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[kalyan kumar kalvagadda via Review Board]

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



Lina, Don't we need changes to notifcation processor to handle this?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
Line 363 (original), 363 (patched)


if changing the database is not supported hive should throw an error, if 
hive allows that command sentry should be handling it.
1. update the path information
2. change the permission


- kalyan kumar kalvagadda


On March 7, 2018, 10:39 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65768/
> ---
> 
> (Updated March 7, 2018, 10:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the bug that moves privilege to wrong table when DB name changes in alter 
> table rename
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  c30d982 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b410027 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
> 
> 
> Diff: https://reviews.apache.org/r/65768/diff/3/
> 
> 
> Testing
> ---
> 
> unit test succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated March 7, 2018, 10:39 p.m.)


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


Repository: sentry


Description
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 7a31a01 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 c30d982 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 b410027 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 c6be80d 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


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

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


Testing
---

unit test succeeded


Thanks,

Na Li

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated March 7, 2018, 7:16 p.m.)


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


Repository: sentry


Description
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 7a31a01 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 b410027 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


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

Changes: https://reviews.apache.org/r/65768/diff/1-2/


Testing
---

unit test succeeded


Thanks,

Na Li