----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57626/#review169238 -----------------------------------------------------------
I like the idea of adding generic parameters to the event. It provides a clean way of sharing information between listeners. I think it would be good to keep all "well-known" parameters in a single class so that it would be immediately clear when there is a name collision. hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java Lines 80 (patched) <https://reviews.apache.org/r/57626/#comment241578> Do you intentionally keep it public? This can be private. Please add comment describing what this is. hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java Line 476 (original), 478 (patched) <https://reviews.apache.org/r/57626/#comment241580> Since you are touching this method, can you add javadoc, explaining what this does and what are parameters? hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java Lines 487 (patched) <https://reviews.apache.org/r/57626/#comment241581> You only need to do this if eventId is set on event. This way we will know exactly whether we have a notification ID for each event or not. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 82 (original), 87 (patched) <https://reviews.apache.org/r/57626/#comment241591> Is there any generic policy about wildcard imports? metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java Lines 138 (patched) <https://reviews.apache.org/r/57626/#comment241582> 1) Naming - it isn't event, it is something rather different 2) It seems that in all contexts wherre this is used we always immediately set environment context and call notify. More over, we check for listeners != null and being non-empty twice - once before we call this and a second time when we call notify. What do you think of adding both environmentContext and listeners to the constructor? Or, even simpler, have a static method which will incorporate all this logic without allocating a new MetaStoreListenerNotifier at all? SOmething like this: public static Optional<Map<String, String>> notifyAll(EventType eventType, ListenerEvent listenerEvent, EnvironmentContext context, final List<MetaStoreEventListener> listeners ) throws MetaException { if (listeners != null && notificationEvents.containsKey(eventType)) { listenerEvent.setEnvironmentContext(context); for (MetaStoreEventListener listener: listeners) { notificationEvents.get(eventType).notify(listener, listenerEvent); } } return listenerEvent.getParameters(); } metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Line 28 (original), 32 (patched) <https://reviews.apache.org/r/57626/#comment241587> Can you document thread safety assumptions of this class (which may be "completely thread unsafe") metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Line 33 (original), 37 (patched) <https://reviews.apache.org/r/57626/#comment241584> Status isn't realy a boolean - it is, essentially an enum, which can only change from Ok to Failure but never another way around. metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 39 (patched) <https://reviews.apache.org/r/57626/#comment241583> Do you actually need the Optional here? Is there a reason simple map which is optionally null wouldn't work? Or, since you always(?) use it, can you just allocate a small map in constructor so that it is never null? metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 60 (patched) <https://reviews.apache.org/r/57626/#comment241585> I think, this should be setFailed() metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 81 (patched) <https://reviews.apache.org/r/57626/#comment241590> Can you add javadoc for all new methods, especially public ones? metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 82 (patched) <https://reviews.apache.org/r/57626/#comment241586> Do we actually need to return these parameters? Do we copy them to other places? Do we need to worry about mutability (e.g. returning a copy of parameters?) metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 86 (patched) <https://reviews.apache.org/r/57626/#comment241588> Do we actually need to explicitely set parameters? metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java Lines 90 (patched) <https://reviews.apache.org/r/57626/#comment241589> Naming - may be addParameter() ? Also - is it allowed to modify existing parameter or not? addParameter() may throw RuntimeException if an attempt is made to modify existing parameter. - Alexander Kolbasov On March 16, 2017, 10:36 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57626/ > ----------------------------------------------------------- > > (Updated March 16, 2017, 10:36 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 > > 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/2/ > > > Testing > ------- > > HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to > complete 100% tests. > > > Thanks, > > Sergio Pena > >
