> 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.
>
> Sergio Pena wrote:
> 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.
Why not do what Alexander suggests. His method signature looks like:
public static Optional<Map<String, String>> notifyAll(EventType eventType,
ListenerEvent
listenerEvent,
EnvironmentContext
context,
final
List<MetaStoreEventListener> listeners
)
So, in other words, no MetaStoreListenerNotifier object is needed.
I don't increasing number of parameters to methods is a bad idea. I find
it more readable -- you know exactly what parameters the method is dealing with.
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57626/#review169320
-----------------------------------------------------------
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
>
>
