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