[jira] [Commented] (BOOKKEEPER-310) Changes in hedwig server to support JMS spec

["Sijie Guo (JIRA)" <ji...@apache.org>]

    [ 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