Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============3588452576739966591=="
MIME-Version: 1.0
Subject: Re: Review Request 57626: HIVE-16164: Provide mechanism for passing
 HMS
 notification ID between transactional and non-transactional listeners.
From: Mohit Sabharwal <mohit@cloudera.com>
To: Sergio Pena <sergio.pena@cloudera.com>, hive <dev@hive.apache.org>,
 Mohit Sabharwal <mohit@cloudera.com>, Alexander Kolbasov <akolb1@gmail.com>
Date: Tue, 21 Mar 2017 20:47:56 -0000
Message-ID: <20170321204756.54987.20763@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Mohit Sabharwal <noreply@reviews.apache.org>
References: <20170317223402.62183.51598@reviews-vm2.apache.org>
In-Reply-To: <20170317223402.62183.51598@reviews-vm2.apache.org>
Reply-To: Mohit Sabharwal <mohit@cloudera.com>
archived-at: Tue, 21 Mar 2017 20:48:08 -0000

--===============3588452576739966591==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============3588452576739966591==--