hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Kolbasov <ako...@gmail.com>
Subject Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.
Date Fri, 17 Mar 2017 01:27:59 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57626/#review169238
-----------------------------------------------------------



I like the idea of adding generic parameters to the event. It provides a clean way of sharing
information between listeners. I think it would be good to keep all "well-known" parameters
in a single class so that it would be immediately clear when there is a name collision.


hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Lines 80 (patched)
<https://reviews.apache.org/r/57626/#comment241578>

    Do you intentionally keep it public? This can be private.
    
    Please add comment describing what this is.



hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Line 476 (original), 478 (patched)
<https://reviews.apache.org/r/57626/#comment241580>

    Since you are touching this method, can you add javadoc, explaining what this does and
what are parameters?



hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Lines 487 (patched)
<https://reviews.apache.org/r/57626/#comment241581>

    You only need to do this if eventId is set on event. This way we will know exactly whether
we have a notification ID for each event or not.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 82 (original), 87 (patched)
<https://reviews.apache.org/r/57626/#comment241591>

    Is there any generic policy about wildcard imports?



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
Lines 138 (patched)
<https://reviews.apache.org/r/57626/#comment241582>

    1) Naming - it isn't event, it is something rather different
    
    2) It seems that in all contexts wherre this is used we always immediately set environment
context and call notify. More over, we check for listeners != null and being non-empty twice
- once before we call this and a second time when we call notify.
    
    What do you think of adding both environmentContext and listeners to the constructor?
Or, even simpler, have a static method which will incorporate all this logic without allocating
a new MetaStoreListenerNotifier at all? SOmething like this:
    
      public static Optional<Map<String, String>> notifyAll(EventType eventType,

                                                            ListenerEvent listenerEvent,
                                                            EnvironmentContext context,
                                                            final List<MetaStoreEventListener>
listeners
                                                            ) throws MetaException {
        if (listeners != null && notificationEvents.containsKey(eventType)) {
          listenerEvent.setEnvironmentContext(context);
          for (MetaStoreEventListener listener: listeners) {
            notificationEvents.get(eventType).notify(listener, listenerEvent);
          }
        }
        
        return listenerEvent.getParameters();
      }



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Line 28 (original), 32 (patched)
<https://reviews.apache.org/r/57626/#comment241587>

    Can you document thread safety assumptions of this class (which may be "completely thread
unsafe")



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Line 33 (original), 37 (patched)
<https://reviews.apache.org/r/57626/#comment241584>

    Status isn't realy a boolean - it is, essentially an enum, which can only change from
Ok to Failure but never another way around.



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 39 (patched)
<https://reviews.apache.org/r/57626/#comment241583>

    Do you actually need the Optional here? Is there a reason simple map which is optionally
null wouldn't work?
    
    Or, since you always(?) use it, can you just allocate a small map in constructor so that
it is never null?



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 60 (patched)
<https://reviews.apache.org/r/57626/#comment241585>

    I think, this should be setFailed()



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 81 (patched)
<https://reviews.apache.org/r/57626/#comment241590>

    Can you add javadoc for all new methods, especially public ones?



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 82 (patched)
<https://reviews.apache.org/r/57626/#comment241586>

    Do we actually need to return these parameters? Do we copy them to other places? Do we
need to worry about mutability (e.g. returning a copy of parameters?)



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 86 (patched)
<https://reviews.apache.org/r/57626/#comment241588>

    Do we actually need to explicitely set parameters?



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 90 (patched)
<https://reviews.apache.org/r/57626/#comment241589>

    Naming - may be addParameter() ?
    Also - is it allowed to modify existing parameter or not?
    addParameter() may throw RuntimeException if an attempt is made to modify existing parameter.


- Alexander Kolbasov


On March 16, 2017, 10:36 p.m., Sergio Pena wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> -----------------------------------------------------------
> 
> (Updated March 16, 2017, 10:36 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 
>   itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java bae39acafeb86d04ac8ec66098be125cd3cef3e0

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