> On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java > > Lines 80 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666416#file1666416line80> > > > > Do you intentionally keep it public? This can be private. > > > > Please add comment describing what this is.
I kept it public so that other listeners, like Sentry, can get the parameter key where the event ID is stored. I moved this constant to another class named MetaStoreEventListenerConstants instead. It is also public for the same reason explained before. > On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 82 (original), 87 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666419#file1666419line87> > > > > Is there any generic policy about wildcard imports? We usually avoid using wildcard imports, but since this HiveMetaStore class is using all of the imported events, I thought keeping it simple was good to have. But let me know why this is not good to do and I will change it. > On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java > > Lines 138 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666420#file1666420line138> > > > > 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(); > > } I thought about this, but I didn't want to do it for two reasons: 1. I wanted to keep the method readable because the number of parameters passed, it is currently 5 (eventType, listenerEvent, context, parameters, listeners) and I don't know if it might increase in the future. 2. I didn't want to create a ListenerEvent object if there was no listeners to notify. I kept thinking on a better wayt for this, but I didn't come with any better method. The above code you wrote looks good, but I'm still thinking on readability and unecessary objects creationg. > On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java > > Lines 82 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666421#file1666421line82> > > > > 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?) I need to return this so we can pass it to the non-transactional listeners. No all places on the HiveMetaStore can re-use the ListenerEvent. I made it immutable now btw. > On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java > > Lines 86 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666421#file1666421line86> > > > > Do we actually need to explicitely set parameters? For the same reason that we need to pass the parameters between transactional and non-transactional listeners because I couldn't reuse the ListenerEvent in all the HiveMetaStore cases. > On March 17, 2017, 1:28 a.m., Alexander Kolbasov wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java > > Lines 90 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666421#file1666421line90> > > > > 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. Yeah, we should do that. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57626/#review169238 ----------------------------------------------------------- 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 > >
