> On March 17, 2017, 10:34 p.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 2476 (original), 2484 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666419#file1666419line2519> > > > > 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.
Agree. If we agree on a good readable way to notify all listeners, then this fire..() method won't be even need it. I'll let you know if that's the case. > On March 17, 2017, 10:34 p.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3020 (patched) > > <https://reviews.apache.org/r/57626/diff/2/?file=1666419#file1666419line3115> > > > > use i for consistency :) The i already existed on the method, and my first intuition was to create a new one. But, the i is not used anymore, so I can reuse it. Thanks. > On March 17, 2017, 10:34 p.m., Mohit Sabharwal 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> > > > > 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. I can avoid the creationg of the new object, and just have a static notifyEvent() method that accepts some parameters, perhaps something like this: if (!listeners.isEmpty()) { CreateTableEvent createTableEvent = new CreateTableEvent(tbl, success, this); createTableEvent.addParameters(transactionalListenerResponses); createTableEvent.setEnvironmentContext(envContext); MetaStoreListenerNotifier .notifyEvent(EventType.CREATE_TABLE, createTableEvent, listeners); } But, I think I still need to check that listeners is not empty before calling the above code to avoid creating the CreateTableEvent object if listeners is empty. I can avoid too much code, and make it cleaner by passing the 2 extra parameters to the notifier, but I still need to create the event. if (!listeners.isEmpty()) { CreateTableEvent createTableEvent = new CreateTableEvent(tbl, success, this); MetaStoreListenerNotifier .notifyEvent(EventType.CREATE_TABLE, createTableEvent, transactionalListenerResponses, envContext, listeners); } I was also thinking on create one method per event, and pass the same parameters, and let the method to create the event only if listeners is not empty. Something like this? MetaStoreListenerNotifier. .onCreateTable(tbl, success, this, transactionalListenerResponses, envContext, listeners); The above code would look cleaner on the HiveMetaStore, but the MetaStoreListenerNotifier must support every event method ,and accept at least 6 parameters like the above code. Sasha, Mohit, which option would you think is ideal? I can also go back to the original code, and keep the for() loops to walk through each listener. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57626/#review169320 ----------------------------------------------------------- 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 > >
