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 Thu, 23 Mar 2017 21:34:22 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message