> 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 135 (patched)
> > <https://reviews.apache.org/r/65704/diff/2/?file=1962959#file1962959line135>
> >
> >     isn't tableEvent.getNewTable() null for any other ALTER event that does 
> > not involve renaming?

It is not null. For example, alter table for adding column command has new 
table set, and it is the same as the old table.


> 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)
> > <https://reviews.apache.org/r/65704/diff/2/?file=1962959#file1962959line137>
> >
> >     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
> 
> Na Li wrote:
>     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?

I will check if those are null and return false


- Na


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


On Feb. 22, 2018, 6:55 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2018, 6:55 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-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> 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
>  9b0aeb2 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/4/
> 
> 
> Testing
> -------
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Reply via email to