[
https://issues.apache.org/jira/browse/HIVE-2147?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13039249#comment-13039249
]
jirapos...@reviews.apache.org commented on HIVE-2147:
-----------------------------------------------------
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. > trunk/metastore/if/hive_metastore.thrift, line 347
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18685#file18685line347>
bq. >
bq. > 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.
bq. >
bq. > This is starting to look like a general purpose messaging/rpc
framework. Is that the intent?
bq. >
bq. > A sendMessage() function with separate request and response message types
should work just as well.
That is correct. But semantically they are different. In sendMessage() user is
just notifying Metastore of an event and is not bothered of return value.
recvMessage() user is asking for a response for his message. This distinction
is further enforced by return types. We could just have one api sendMessage()
for both as you suggested, but having distinct apis for sending and receiving
makes it easier for client to understand the semantics.
bq. > This is starting to look like a general purpose messaging/rpc framework.
Well general purpose rpc framework would be much more sophisticated. I am not
aiming for that.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. > trunk/metastore/if/hive_metastore.thrift, line 348
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18685#file18685line348>
bq. >
bq. > 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.
There are two other alternatives that I thought of before settling on this one.
1) Add specific apis for different message types. This would have made doing
this generic api redundant but then this will result in application specific
apis in the metastore. E.g., in HCatalog we want to send a message for "set of
partitions" telling Metastore to mark them as done. What does
finalizePartition() mean in metastore api when Metastore itself is not aware of
this concept as this is application specific. This would be confusing.
2) Use enums instead of integer. This will result in similar problem as above
though on a lower scale. Enums give compile time safety so we have to define
them in Metastore code. Defining application specific enums doesnt look like a
good idea because of similar reasons.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
line 3126
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18694#file18694line3126>
bq. >
bq. > So the event model is that each event may be handled by at most one
event handler?
Yes.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
line 3134
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18694#file18694line3134>
bq. >
bq. > Please add some DEBUG or TRACE level logging here that indicates
which handler consumed a particular event, or if an event was unserviceable.
Will add logging.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
line 3149
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18694#file18694line3149>
bq. >
bq. > Semantically this function looks more like "sendRequest" than
"receiveMessage" (and "sendMessage" looks more like "fireEvent").
Same as my very first comment.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java,
line 3151
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18694#file18694line3151>
bq. >
bq. > Checkstyle: you need a space between control flow tokens and open
parens.
bq. >
will roll this in.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java,
line 640
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18696#file18696line640>
bq. >
bq. > Nice to have: javadoc.
Will add.
bq. On 2011-05-25 03:43:30, Carl Steinbach wrote:
bq. >
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java,
line 86
bq. > <https://reviews.apache.org/r/738/diff/1/?file=18697#file18697line86>
bq. >
bq. > canProcessSendMessage() looks like a redundant call. Is there any
reason that this can't be be rolled into processSendMessage()?
bq. >
Event model is every event is handled by atmost one handler. If we roll this in
processSendMsg() then we have to make this method return boolean which will
tell whether this event got serviced by this handler or not. Then how will it
communicate back the actual return value. In case of sendMsg() this is fine,
but recvMsg() returns a valid value which is then need to be returned to a
client. So, we first ask handler if it can handle the message and then expect a
valid return value in processRecvMsg() call.
- Ashutosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/738/#review713
-----------------------------------------------------------
On 2011-05-12 21:03:29, Ashutosh Chauhan wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/738/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-05-12 21:03:29)
bq.
bq.
bq. Review request for hive and Carl Steinbach.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Updated patch to include missing ASF license and generated thrift code.
bq.
bq.
bq. This addresses bug HIVE-2147.
bq. https://issues.apache.org/jira/browse/HIVE-2147
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. trunk/metastore/if/hive_metastore.thrift 1102450
bq. trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1102450
bq. trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1102450
bq.
trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp
1102450
bq.
trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java
1102450
bq.
trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php
1102450
bq.
trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote
1102450
bq.
trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py
1102450
bq. trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1102450
bq.
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
1102450
bq.
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
1102450
bq.
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
1102450
bq.
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
1102450
bq.
trunk/metastore/src/java/org/apache/hadoop/hive/metastore/events/MessageEvent.java
PRE-CREATION
bq.
trunk/metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java
1102450
bq.
trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
1102450
bq.
bq. Diff: https://reviews.apache.org/r/738/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated TestMetaStoreEventListener to test new api.
bq.
bq.
bq. Thanks,
bq.
bq. Ashutosh
bq.
bq.
> Add api to send / receive message to metastore
> ----------------------------------------------
>
> Key: HIVE-2147
> URL: https://issues.apache.org/jira/browse/HIVE-2147
> Project: Hive
> Issue Type: Improvement
> Components: Metastore
> Reporter: Ashutosh Chauhan
> Assignee: Ashutosh Chauhan
> Fix For: 0.8.0
>
> Attachments: api-without-thrift.patch
>
>
> This is follow-up work on HIVE-2038.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
