[
https://issues.apache.org/jira/browse/BOOKKEEPER-310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409233#comment-13409233
]
Sijie Guo commented on BOOKKEEPER-310:
--------------------------------------
reviews for hedwig to support returning message seq id for publish requests are
listed as below:
1) protocol changes in BOOKKEEPER-309:
{code}
+ public static PubSubResponse getSuccessResponse(long txnId,
PubSubProtocol.MessageSeqId publishedMessageSeqId) {
+ if (null == publishedMessageSeqId) return
getBasicBuilder(StatusCode.SUCCESS).setTxnId(txnId).build();
+ PubSubProtocol.PublishResponse publishResponse =
PubSubProtocol.PublishResponse.newBuilder().setPublishedMsgId(publishedMessageSeqId).build();
+ PubSubProtocol.ResponseBody responseBody =
PubSubProtocol.ResponseBody.newBuilder().setPublishResponse(publishResponse).build();
+ return
getBasicBuilder(StatusCode.SUCCESS).setTxnId(txnId).setResponseBody(responseBody).build();
+ }
+
{code}
I would be better to provide a common help method to convert ResponseBody to
PubSubResponse. And for PublishResponse, there is special help method to form
PublishResponse. It might be:
{code}
// convert response body to pubsub response.
public static PubSubResponse getSuccessResponse(long txnId,
PubSubProtocol.ResponseBody respBody);
// form publish response
public static PubSubProtocol.ResponseBody.Builder
getPublishResponse(PubSubProtocol.MessageSeqId publishedMessageSeqId);
{code}
2) server changes in BOOKKEEPER-310:
{code}
- ++latencyBuckets[bucket];
+ // This threw java.lang.ArrayIndexOutOfBoundsException: for me in
a test.
+ if (bucket < latencyBuckets.length) ++latencyBuckets[bucket];
{code}
the ArrayIndexOutOfBoundsException is due to time changed, so a negative
latency is passed. it would be fixed in BOOKKEEPER-327 and BOOKKEEPER-330 for
both hedwig and bookkeeper.
{code}
message.getBody();
+ // Optional to have body : not sure why the dangling call
exists ! Flush state ?
+ // if (message.hasBody()) message.getBody();
{code}
seems body is a required field for Message. is there anything special you
considered?
> Changes in hedwig server to support JMS spec
> --------------------------------------------
>
> Key: BOOKKEEPER-310
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-310
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-server.patch, hedwig-server.patch.1
>
>
> The primary changes are :
> a) Support modified protocol changes (optional body).
> b) Return the published message's seq-id in the response.
> c) Minor bugfix to Array indexing in bucket which was triggered in a testcase.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
