hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mohit Sabharwal <mo...@cloudera.com>
Subject Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.
Date Tue, 21 Mar 2017 20:47:56 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message