> 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?
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). - 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 > >
