hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mohit Sabharwal <mo...@cloudera.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 22:34:02 GMT

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




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

    I'd call this DB_NOTIFICATION_EVENT_ID_KEY_NAME, since this is not the id, but a key to
find the id.



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

    This really clutters the namespace, so please don't remove the wildcard, even if every
class is needed.



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

    I like the idea of keeping fireMetaStoreAddPartitionEventTransactional method - because
it makes it more readable that way. Otherwise, very similar looking code is duplicated 7 times.

    
    You can have fireMetaStoreAddPartitionEventTransactional return transactionalListenerResponses
for later re-use.  But I wouldn't inline this code directly in add_partitions_core for readability
reasons.



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

    use i for consistency :)



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

    extra line



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

    I agree with Alexander. We can avoid code duplication by doing most work inside notify.
    
    I also agree that a simple static method is going to be much cleaner than MetaStoreListenerNotifier
in this use case. 
    
    The point of builder-like pattern (like MetaStoreListenerNotifier) to make it cleaner
to construct an object. But in this case, we are introducing a new object when none is really
needed. A simple static method is going to way more readable.
    
    Even cost-wise the branch (listeners != null) is expensive, so let's not do it twice.


- Mohit Sabharwal


On March 17, 2017, 8:14 p.m., Sergio Pena wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> -----------------------------------------------------------
> 
> (Updated March 17, 2017, 8:14 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 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/3/
> 
> 
> 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