> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > This is good, but I am wondering what is the purpose in using different
> > events between transactional and non-transactional listeners in most cases?
> > It would be much simpler and cleaner to reuse existing event except for one
> > or two cases where this isn't possible.
I don't think it would be simpler. Hive could have both or either one of
transactional or non-transactoinal listeners enabled. If transactional
listeners are not enabled but listeners, then we have to create the event
later. If none of listeners are enabled, then the event should not be created.
This will another IF condition.
I.e.
CreateDatabaseEvent event = null;
try {
if (!transactionalListeners.isEmpty()) {
event = new CreateDatabaseEvent(...);
event.setEnvironmentContext(...);
call transactionalListeners
}
} catch (...) {
if (listeners.isEmpty()) {
if (event == null) {
event = new CreateDatabaseEvent(...);
event.setEnvironmentContext(...);
}
call listeners
}
}
The above code can reuse the CreateDatabaseEvent, but we still need to create
it if transactionalListeners was disabled. Also, we would need to add code to
all events to accept the setStatus() method in case we're reusing the event.
That's why I just remove that and keep it simple by leaving the old way to
recreate the event.
try {
if (!transactionalListeners.isEmpty()) {
MetaStoreListenerNotifier
.newNotification(EventType.CREATE_DATABASE, new
CreateDatabaseEvent(db, true, this))
.setEnvironmentContext(environmentContext).notify(transactionalListeners);
}
} catch (...) {
if (listeners.isEmpty()) {
MetaStoreListenerNotifier
.newNotification(EventType.CREATE_DATABASE, new
CreateDatabaseEvent(db, success, this))
.setEnvironmentContext(environmentContext).notify(listeners);
}
}
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 2491 (original), 2526 (patched)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670533#file1670533line2535>
> >
> > Why is this the case? The above condition is never null.
Right, but it can be empty.
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2530 (patched)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670533#file1670533line2539>
> >
> > So why do we notify them twice - once with success and another time
> > with failure? Something is wrong here.
I don't know the idea behind this notification. Maybe someone else is
processing failed events for logging or something. I don't want to remove it
because of legacy problems.
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 2820 (original)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670533#file1670533line2891>
> >
> > Just to make sure - this is kind of independent cleanup - right?
Yes. I am trying to coming up with a better approach for this cleanup.
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
> > Lines 195 (patched)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670534#file1670534line195>
> >
> > In all places where you call it, you know that listeners != null &&
> > !listeners.empty()
> >
> > So may be this should be replaced with preconditions check?
That would be a good thing. I'm still figuring out a better way to improve the
notifier to avoid checking IF() twice (one outside the class, and another in
this class). But PreCondition is a good one as this class is just an internal
API.
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
> > Lines 120 (patched)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670535#file1670535line120>
> >
> > Loking at the way it is used, I would consider adding constructor with
> > parameters instead. They are always used like this:
> >
> >
> > if (!listeners.isEmpty()) {
> > MetaStoreListenerNotifier
> > .newNotification(EventType.CREATE_INDEX, new
> > AddIndexEvent(index, success, this))
> > .addParameters(transactionalListenerResponses)
> > .notify(listeners);
> > }
> >
> > This is essentially equivalent to constructing a new event with the
> > given set of parameters.
They're always used only on the non-transactional listeners but
transactionalListeners. I will think about another static method to pass all
parameters instead.
> On March 23, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
> > Lines 121 (patched)
> > <https://reviews.apache.org/r/57626/diff/4/?file=1670535#file1670535line121>
> >
> > If you decide to keep this interface,it would be cleaner to require
> > that parameters isn't null and verify with Preconditions.checkNotNull
This is a public method that may be called by external listeners. I'd like to
avoid throwing an exception and message if the parameters is null. I like that
idea of just ignoring the null and not adding anything to the parameters.
That's why I used the 'addParameters' name instead of 'setParameter' to giving
a meaning.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57626/#review169801
-----------------------------------------------------------
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
>
>
