----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57626/#review169320 -----------------------------------------------------------
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java Lines 80 (patched) <https://reviews.apache.org/r/57626/#comment241665> I'd call this DB_NOTIFICATION_EVENT_ID_KEY_NAME, since this is not the id, but a key to find the id. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 82 (original), 87 (patched) <https://reviews.apache.org/r/57626/#comment241666> This really clutters the namespace, so please don't remove the wildcard, even if every class is needed. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 2476 (original), 2484 (patched) <https://reviews.apache.org/r/57626/#comment241674> I like the idea of keeping fireMetaStoreAddPartitionEventTransactional method - because it makes it more readable that way. Otherwise, very similar looking code is duplicated 7 times. You can have fireMetaStoreAddPartitionEventTransactional return transactionalListenerResponses for later re-use. But I wouldn't inline this code directly in add_partitions_core for readability reasons. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3020 (patched) <https://reviews.apache.org/r/57626/#comment241694> use i for consistency :) metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3308 (patched) <https://reviews.apache.org/r/57626/#comment241695> extra line metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java Lines 138 (patched) <https://reviews.apache.org/r/57626/#comment241699> I agree with Alexander. We can avoid code duplication by doing most work inside notify. I also agree that a simple static method is going to be much cleaner than MetaStoreListenerNotifier in this use case. The point of builder-like pattern (like MetaStoreListenerNotifier) to make it cleaner to construct an object. But in this case, we are introducing a new object when none is really needed. A simple static method is going to way more readable. Even cost-wise the branch (listeners != null) is expensive, so let's not do it twice. - Mohit Sabharwal On March 17, 2017, 8:14 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57626/ > ----------------------------------------------------------- > > (Updated March 17, 2017, 8:14 p.m.) > > > Review request for hive. > > > Bugs: HIVE-16164 > https://issues.apache.org/jira/browse/HIVE-16164 > > > Repository: hive-git > > > Description > ------- > > This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID > property from withing the DbNotificationListener class. It then passes the > EnvironmentContext from transactional listeners to non-transactional > listeners so that the eventId is shared between them. > > The patch provides the following changes: > - DbNotificationListener Changes to pass the EnvironmentContext from > transactional to non-transactional listeners. > - HiveAlterHandler Changes to pass the EnvironmentContext from > transactional to non-transactional listeners. > - MetaStoreListenerNotifier New helper class that wraps the notification > call to the listeners. > - TestObjectStore Verifies that the addNotificationEvent() > method saves the eventId on the NotificationEvent object. > - TestDbNotificationListener Verifies that any HMS call is passing the > DB_NOTIFICATION_EVENT_ID to non-transactional listeners. > > > Diffs > ----- > > > hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java > f7e3e3a0a71094992fdf4bd3ceea2da0bf7d1ff0 > > hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java > PRE-CREATION > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java > 1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a > metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > bae39acafeb86d04ac8ec66098be125cd3cef3e0 > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 07eca38190c1b05bb4a3977e9154423449828957 > > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java > PRE-CREATION > > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java > 62aeb8cc343fc4aab72e76da890e36ac3a16b7bf > metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java > 1f87eeb18f6edf7351b3c8da6a6826c08656e48c > > > Diff: https://reviews.apache.org/r/57626/diff/3/ > > > Testing > ------- > > HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to > complete 100% tests. > > > Thanks, > > Sergio Pena > >
