----------------------------------------------------------- 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) <https://reviews.apache.org/r/65704/#comment278169> 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) <https://reviews.apache.org/r/65704/#comment278171> 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) <https://reviews.apache.org/r/65704/#comment278173> 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) <https://reviews.apache.org/r/65704/#comment278172> 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) <https://reviews.apache.org/r/65704/#comment278168> 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 > >
