You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Ashutosh Chauhan <ha...@apache.org> on 2011/04/18 20:44:05 UTC

Review Request: Addressing Carl's comments.

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

Review request for hive and Carl Steinbach.


Summary
-------

Addresses Carl's comment for previous patch. 
Also added a new method finalizePartition in metastore through which metastore client can indicate to metastore that partition can be finalized.


This addresses bug HIVE-2038.
    https://issues.apache.org/jira/browse/HIVE-2038


Diffs
-----

  trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1094688 
  trunk/conf/hive-default.xml 1094688 
  trunk/metastore/if/hive_metastore.thrift 1094688 
  trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1094688 
  trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1094688 
  trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp 1094688 
  trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 1094688 
  trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php 1094688 
  trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote 1094688 
  trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py 1094688 
  trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1094688 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1094688 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 1094688 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 1094688 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java PRE-CREATION 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 1094688 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/NoOpListener.java PRE-CREATION 
  trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1094688 
  trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java PRE-CREATION 
  trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java PRE-CREATION 

Diff: https://reviews.apache.org/r/618/diff


Testing
-------


Thanks,

Ashutosh

Re: Review Request: Addressing Carl's comments.

Posted by Carl Steinbach <ca...@cloudera.com>.

> On 2011-04-21 22:51:56, Carl Steinbach wrote:
> > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java, line 41
> > <https://reviews.apache.org/r/618/diff/1/?file=15927#file15927line41>
> >
> >     Instead of passing in raw Table/Partition/Database objects please wrap these objects in containers, e.g. CreateTableEvent, DropTableEvent, etc.
> >     
> >     Q: Whats the advantage of wrapper container objects?
> >     
> >     The advantage of container objects is that it allows us to evolve the API without breaking compatibility with older clients. With the current interface if I want to add a parameter to one of these methods I will break compatibility, but if we use container objects I can add a new field to the container without affecting older clients.

It seems like this same concern also dictates that MetaStoreListener should be an abstract class instead of an interface. Can you please make this change?


- Carl


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


On 2011-04-18 18:44:05, Ashutosh Chauhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/618/
> -----------------------------------------------------------
> 
> (Updated 2011-04-18 18:44:05)
> 
> 
> Review request for hive and Carl Steinbach.
> 
> 
> Summary
> -------
> 
> Addresses Carl's comment for previous patch. 
> Also added a new method finalizePartition in metastore through which metastore client can indicate to metastore that partition can be finalized.
> 
> 
> This addresses bug HIVE-2038.
>     https://issues.apache.org/jira/browse/HIVE-2038
> 
> 
> Diffs
> -----
> 
>   trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1094688 
>   trunk/conf/hive-default.xml 1094688 
>   trunk/metastore/if/hive_metastore.thrift 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 1094688 
>   trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py 1094688 
>   trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/NoOpListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1094688 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java PRE-CREATION 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/618/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ashutosh
> 
>

Re: Review Request: Addressing Carl's comments.

Posted by Carl Steinbach <ca...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/618/#review522
-----------------------------------------------------------



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/618/#comment1070>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/618/#comment1071>

    formatting



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/618/#comment1072>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/618/#comment1073>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/618/#comment1074>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
<https://reviews.apache.org/r/618/#comment1075>

    This change should go in a separate ticket. What does it mean to "finalize" a partition?



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1076>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1087>

    Instead of passing in raw Table/Partition/Database objects please wrap these objects in containers, e.g. CreateTableEvent, DropTableEvent, etc.
    
    Q: Whats the advantage of wrapper container objects?
    
    The advantage of container objects is that it allows us to evolve the API without breaking compatibility with older clients. With the current interface if I want to add a parameter to one of these methods I will break compatibility, but if we use container objects I can add a new field to the container without affecting older clients.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1077>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1078>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1079>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1080>

    Please remove this and include it in a separate patch. We need to discuss what "finalizing" a partition actually means, and how it relates to locking.



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
<https://reviews.apache.org/r/618/#comment1083>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
<https://reviews.apache.org/r/618/#comment1082>

    spacing



trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
<https://reviews.apache.org/r/618/#comment1081>

    spacing



trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
<https://reviews.apache.org/r/618/#comment1086>

    spacing


- Carl


On 2011-04-18 18:44:05, Ashutosh Chauhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/618/
> -----------------------------------------------------------
> 
> (Updated 2011-04-18 18:44:05)
> 
> 
> Review request for hive and Carl Steinbach.
> 
> 
> Summary
> -------
> 
> Addresses Carl's comment for previous patch. 
> Also added a new method finalizePartition in metastore through which metastore client can indicate to metastore that partition can be finalized.
> 
> 
> This addresses bug HIVE-2038.
>     https://issues.apache.org/jira/browse/HIVE-2038
> 
> 
> Diffs
> -----
> 
>   trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1094688 
>   trunk/conf/hive-default.xml 1094688 
>   trunk/metastore/if/hive_metastore.thrift 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 1094688 
>   trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py 1094688 
>   trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/NoOpListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1094688 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java PRE-CREATION 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/618/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ashutosh
> 
>

Re: Review Request: Addressing Carl's comments.

Posted by Carl Steinbach <ca...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/618/#review526
-----------------------------------------------------------



trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/618/#comment1090>

    I think it should be possible to specify multiple metastore listeners. Can you please make the following changes:
    
    * Change the name of the conf property from "hive.metastore.listener" to "hive.metastore.event.listeners" and update the description to indicate that this is a comma separated list of listener classnames.
    
    * Change the default value for this property to be an empty string.
    
    * Update the code that initializes these listeners. You can cut and paste the code that's used to initialize hive.exec.pre.hooks
    


- Carl


On 2011-04-18 18:44:05, Ashutosh Chauhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/618/
> -----------------------------------------------------------
> 
> (Updated 2011-04-18 18:44:05)
> 
> 
> Review request for hive and Carl Steinbach.
> 
> 
> Summary
> -------
> 
> Addresses Carl's comment for previous patch. 
> Also added a new method finalizePartition in metastore through which metastore client can indicate to metastore that partition can be finalized.
> 
> 
> This addresses bug HIVE-2038.
>     https://issues.apache.org/jira/browse/HIVE-2038
> 
> 
> Diffs
> -----
> 
>   trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1094688 
>   trunk/conf/hive-default.xml 1094688 
>   trunk/metastore/if/hive_metastore.thrift 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp 1094688 
>   trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 1094688 
>   trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote 1094688 
>   trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py 1094688 
>   trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 1094688 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/NoOpListener.java PRE-CREATION 
>   trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1094688 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java PRE-CREATION 
>   trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/618/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ashutosh
> 
>