hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergio Pena <sergio.p...@cloudera.com>
Subject Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.
Date Mon, 03 Apr 2017 15:59:37 GMT


> On March 30, 2017, 1:54 p.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
> > Lines 51-52 (patched)
> > <https://reviews.apache.org/r/57626/diff/6/?file=1677663#file1677663line51>
> >
> >     What's the point of have duplicate parameter map ? What is the use case ?
> >     
> >     If you need unmodifiable paramters, why can't you add a new method called getunmodifiableParameters()
which simply return Collections.unmodifiableMap(parameters) ?
> >     
> >     Do you really need to call updateUnmodifiableParameters() everytime ? Looks
like you're only updating 'parameters' variable, but then creating a new updateUnmodifiableParameters
every time a new items is added. 
> >     
> >     As I said, you can simply add a new method called getunmodifiableParameters()
which returns Collections.unmodifiableMap(parameters) without the need for this copy.
> 
> Sergio Pena wrote:
>     We want to avoid that an external listener gets a reference to the mutable map object
and override any key/value already set by other listeners. Part of the contract on the ListenerEvent.putParameter()
is that you cannot override an existing key/value. Because we don't control the external listeners,
we prefered to return an inmutable map whenever getParameters() was called.
>     
>     Does it make sense?
> 
> Mohit Sabharwal wrote:
>     That makes complete sense. But why do you need two explicit references to the same
map stored as class members ? You could just have one reference called 'parameters' and add
a getter (called, say, getunmodifiableParameters) that returns Collections.unmodifiableMap(parameters).
> 
> Sergio Pena wrote:
>     I did that to avoid having a "live" unmodifiable map in case the ListenerEvent was
called by multiple threads. But, I don't think that would happen. I even documented the class
to state that this was not supposed to be called by multiple threads. So, I think it make
sense just using Collections.unmodifiableMap(parameters) directly  to avoid making a copy
of the mutable reference for every new key/value pair.

Mohit, forget what I said on the above response. I had in my mind I was doing a copy of mutable
map values to inmutable maps, but I see I am only wrapping the mutable map into the inmutable
map.

Anyways, this is what Sasha commented last time when I had the following code:

    public final Map<String, String> getParameters() {
       return Collections.unmodifiableMap(parameters);
    }
    
    Sasha comment:
    This is fine conceptually, but I'm a bit concerned about performance - there are many
gets (for each call to notify()) and each one has to create a new copy of the map.
    If you want to preserve the code structure (many getters), I would suggest caching an
unmodifiable map when it is updated since updates are rare.
    
So far we only have 1 put on the DbNotificationListener. After that, many events can call
the getParameters(), and we won't know how they handle that, if storing a reference of the
inmutable parameters once, or if they like to do 'event.getParameters().get(key)' many times.
I thing that's what Sasha tried to avoid.


- Sergio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57626/#review170576
-----------------------------------------------------------


On March 28, 2017, 3:17 p.m., Sergio Pena wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> -----------------------------------------------------------
> 
> (Updated March 28, 2017, 3:17 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
ea6cb790bc03fc38be07a3be7be34aa9c5d293ba 
>   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 80b1e98b9efbf068f0cd32303499a3a9b5bbd67b

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