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 Thu, 23 Mar 2017 00:27:39 GMT

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



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.


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

    This line is rather long - can you break it?



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

    Please document the semantics of this key - what it means and what is the supposed value.



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

    It can never be null, so you can remove the if part.



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

    Why is this the case? The above condition is never null.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2530 (patched)
<https://reviews.apache.org/r/57626/#comment242439>

    So why do we notify them twice - once with success and another time with failure? Something
is wrong here.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 2820 (original)
<https://reviews.apache.org/r/57626/#comment242440>

    Just to make sure - this is kind of independent cleanup - right?



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2961 (patched)
<https://reviews.apache.org/r/57626/#comment242441>

    We know the size upfront - it is partitionsToExchange.size(), so it is better to specify

    
    ... = new ArrayList<>(partitionsToExchange.size()



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

    Note that if one of the transactional listeners failed, the length of the parameters list
may be smaller then the length of partitionsToExchange and you'll get out of bounds error
when you call get(i)



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

    Unusual comment formatting - the normal style is 
    
    /*
     * some text
     * more text
     */



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

    'Helper class' doesn't explain much - can you explain the real purpose in life of this
class?



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

    In all places where you call it, you know that listeners != null && !listeners.empty()
    
    So may be this should be replaced with preconditions check?



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

    Typo: parameters *are* not thread-safe. 
    
    Also you allow parameters to be exposed outside or replaced - it is nice to document this.



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

    Since you know that this map is usually very small, it makes sense to provide an initial
size to override default (e.g. 1 or 2).



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

    s/list/map



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

    The fact that no parameters means an empty map is kind of obvious - the non-obvious thing
is that you guarantee that you never return null.



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

    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.



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

    rewording the message: something like
    "Invalid attempt to overwrite read-only parameter ' + name



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

    Please document that a parameter can only be set once.
    
    Document that none of the parameters should exist.



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

    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.



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

    If you decide to keep this interface,it would be cleaner to require that parameters isn't
null and verify with Preconditions.checkNotNull


- Alexander Kolbasov


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