----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57626/#review169801 -----------------------------------------------------------
This is good, but I am wondering what is the purpose in using different events between transactional and non-transactional listeners in most cases? It would be much simpler and cleaner to reuse existing event except for one or two cases where this isn't possible. hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java Lines 494 (patched) <https://reviews.apache.org/r/57626/#comment242433> This line is rather long - can you break it? hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java Lines 28 (patched) <https://reviews.apache.org/r/57626/#comment242435> Please document the semantics of this key - what it means and what is the supposed value. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 2490 (original), 2525 (patched) <https://reviews.apache.org/r/57626/#comment242437> It can never be null, so you can remove the if part. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 2491 (original), 2526 (patched) <https://reviews.apache.org/r/57626/#comment242438> Why is this the case? The above condition is never null. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 2530 (patched) <https://reviews.apache.org/r/57626/#comment242439> So why do we notify them twice - once with success and another time with failure? Something is wrong here. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 2820 (original) <https://reviews.apache.org/r/57626/#comment242440> Just to make sure - this is kind of independent cleanup - right? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 2961 (patched) <https://reviews.apache.org/r/57626/#comment242441> We know the size upfront - it is partitionsToExchange.size(), so it is better to specify ... = new ArrayList<>(partitionsToExchange.size() metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 3006 (original), 3028 (patched) <https://reviews.apache.org/r/57626/#comment242442> Note that if one of the transactional listeners failed, the length of the parameters list may be smaller then the length of partitionsToExchange and you'll get out of bounds error when you call get(i) metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 6535 (original), 6579 (patched) <https://reviews.apache.org/r/57626/#comment242443> Unusual comment formatting - the normal style is /* * some text * more text */ metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java Lines 47 (patched) <https://reviews.apache.org/r/57626/#comment242445> 'Helper class' doesn't explain much - can you explain the real purpose in life of this class? metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java Lines 195 (patched) <https://reviews.apache.org/r/57626/#comment242444> In all places where you call it, you know that listeners != null && !listeners.empty() So may be this should be replaced with preconditions check? metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 46 (patched) <https://reviews.apache.org/r/57626/#comment242415> Typo: parameters *are* not thread-safe. Also you allow parameters to be exposed outside or replaced - it is nice to document this. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 57 (patched) <https://reviews.apache.org/r/57626/#comment242416> Since you know that this map is usually very small, it makes sense to provide an initial size to override default (e.g. 1 or 2). metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 92 (patched) <https://reviews.apache.org/r/57626/#comment242417> s/list/map metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 95 (patched) <https://reviews.apache.org/r/57626/#comment242419> The fact that no parameters means an empty map is kind of obvious - the non-obvious thing is that you guarantee that you never return null. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 98 (patched) <https://reviews.apache.org/r/57626/#comment242418> This is fine conceptually, but I'm a bit concerned about performance - there are many gets (for each call to notify()) and each one has to create a new copy of the map. If you want to preserve the code structure (many getters), I would suggest caching an unmodifiable map when it is updated since updates are rare. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 109 (patched) <https://reviews.apache.org/r/57626/#comment242422> rewording the message: something like "Invalid attempt to overwrite read-only parameter ' + name metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 116 (patched) <https://reviews.apache.org/r/57626/#comment242421> Please document that a parameter can only be set once. Document that none of the parameters should exist. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 120 (patched) <https://reviews.apache.org/r/57626/#comment242427> Loking at the way it is used, I would consider adding constructor with parameters instead. They are always used like this: if (!listeners.isEmpty()) { MetaStoreListenerNotifier .newNotification(EventType.CREATE_INDEX, new AddIndexEvent(index, success, this)) .addParameters(transactionalListenerResponses) .notify(listeners); } This is essentially equivalent to constructing a new event with the given set of parameters. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 121 (patched) <https://reviews.apache.org/r/57626/#comment242423> If you decide to keep this interface,it would be cleaner to require that parameters isn't null and verify with Preconditions.checkNotNull - Alexander Kolbasov On March 20, 2017, 10:33 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57626/ > ----------------------------------------------------------- > > (Updated March 20, 2017, 10:33 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 > 4ce6a659c94265d83abbaedb9c52d7e15bf8dde6 > 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/4/ > > > Testing > ------- > > HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to > complete 100% tests. > > > Thanks, > > Sergio Pena > >
