> 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. > > Sergio Pena wrote: > 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.
ok, if performance is the concern, we need to add comments, because it's just confusing otherwise. - Mohit ----------------------------------------------------------- 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 > >
