----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/738/#review713 -----------------------------------------------------------
trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/738/#comment1428> Having separate calls for sending request and response messages looks unnecessary. A sendMessage() function with separate request and response message types should work just as well, and will help to avoid confusion -- otherwise I think people will assume that receiveMessage is a polling call. This is starting to look like a general purpose messaging/rpc framework. Is that the intent? trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/738/#comment1429> Identifying the message type using an integer seems brittle. This won't work if you have more than one application that is firing events at the metastore. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/738/#comment1430> So the event model is that each event may be handled by at most one event handler? trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/738/#comment1431> Please add some DEBUG or TRACE level logging here that indicates which handler consumed a particular event, or if an event was unserviceable. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/738/#comment1432> Semantically this function looks more like "sendRequest" than "receiveMessage" (and "sendMessage" looks more like "fireEvent"). trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/738/#comment1433> Checkstyle: you need a space between control flow tokens and open parens. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java <https://reviews.apache.org/r/738/#comment1434> Nice to have: javadoc. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java <https://reviews.apache.org/r/738/#comment1435> canProcessSendMessage() looks like a redundant call. Is there any reason that this can't be be rolled into processSendMessage()? - Carl On 2011-05-12 21:03:29, Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/738/ > ----------------------------------------------------------- > > (Updated 2011-05-12 21:03:29) > > > Review request for hive and Carl Steinbach. > > > Summary > ------- > > Updated patch to include missing ASF license and generated thrift code. > > > This addresses bug HIVE-2147. > https://issues.apache.org/jira/browse/HIVE-2147 > > > Diffs > ----- > > trunk/metastore/if/hive_metastore.thrift 1102450 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1102450 > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1102450 > > trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > 1102450 > > trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 1102450 > > trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php > 1102450 > > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > 1102450 > trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 1102450 > trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1102450 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 1102450 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 1102450 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 1102450 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java > 1102450 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/MessageEvent.java > PRE-CREATION > > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java > 1102450 > > trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java > 1102450 > > Diff: https://reviews.apache.org/r/738/diff > > > Testing > ------- > > Updated TestMetaStoreEventListener to test new api. > > > Thanks, > > Ashutosh > >
