You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by "Mridul Muralidharan (JIRA)" <ji...@apache.org> on 2012/06/21 23:09:42 UTC
[jira] [Created] (BOOKKEEPER-311) Protocol changes in hedwig client
api to support JMS spec
Mridul Muralidharan created BOOKKEEPER-311:
----------------------------------------------
Summary: Protocol changes in hedwig client api to support JMS spec
Key: BOOKKEEPER-311
URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
Project: Bookkeeper
Issue Type: Sub-task
Reporter: Mridul Muralidharan
Primary changes are :
a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
In addition, there are also fixes for
d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424749#comment-13424749 ]
Flavio Junqueira commented on BOOKKEEPER-311:
---------------------------------------------
The Sun conventions say that lines shouldn't be wider than 80 characters:
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#313
But, given that we haven't been following the formatting strictly, I don't feel I can hold this patch any longer, so it is good for me, +1.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13420570#comment-13420570 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
@Flavio,
>> There is one TODO in the patch
Created BOOKKEEPER-350 for it and marked it as dependency for BOOKKEEPER-312.
>> This patch seems to introduce a discrepancy between the api of the java and of the C clients.
BOOKKEEPER-339 is created to support returning message seq id for cpp client.
Since BOOKKEEPER-350 and BOOKKEEPER-331 has been created to track two requirements for JMS provider, I would move BOOKKEEPER-309, BOOKKEEPER-310, BOOKKEEPER-311 as sub-tasks for BOOKKEEPER-331.
>> FORMAT.
Agreed. @Mridul, could you provide a new patch according to BookKeeper's code style.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409467#comment-13409467 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
thanks for Mridul for explaining details.
from your explanation, actually what you need is different ack guarantees. so we could categorized the semantics of consume into several groups:
1) just guarantee sending consume request to local netty layer : if channel is closed before consume request written to server by netty, the consume request is lost. we would get duplicated messages.
2) just guarantee writing consume request to channel : if hub server is down before processing the consume request. we still get duplicated messages.
3) guarantee consume request is processed by hub server : we need response for consume request. in reality, we might not need such guarantee. I wrote it here, is just for completeness.
so I think the better idea is to extend 'consume' api to support different-level consume guarantee, instead of introducing 'asyncConsume' api. I wrote an initial idea as below:
{code}
enum ConsumeMode {
// provide different guarantee levels as described above.
// I had no good names for it.
}
public void consume(topic, subscriberId, messageSeqId);
public void consume(topic, subscriberId, messageSeqId, ConsumeMode);
{code}
the original consume api still keeps semantic just sending consume requests to netty layer. we don't need to break the backward compatibility. and it makes semantic more clearly and extensible. also it could resolve the issue you mentioned.
@Mridul @Ivan, how are your opinions?
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client.patch.3
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409570#comment-13409570 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
I don't think it's inconsistent between apis. the usage of consume is to ack hub server when it received and processed a message. if you treated message delivery as a kind of request, consume could be treated as its response, which is a kind of server->client operation. from this side, it is different from pub/sub/unsub requests, which are client->server operations. also for pub/sub/unsub, we had to receive the response even using asynchronized api, which is the semantic require for these actions.
{quote}
Having said that, as long as there is reasonable assurance that best case effort was made to send consume request to server, any additional guarantees would be better
{quote}
I agreed. but for some proxy-style use cases, there are several running servers using hedwig client interacting with hub servers. so the connection would not be broken until it is down, so netty could handle sending consume request for it without blocking its other logic. why not leverage it?
yes. for auto-consume clients, it is OK to provide better guarantee as it could. but for those clients who consume themselves, they could choose how to ack hub server as original api provided.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client.patch
Implement the changes specified in the description
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409517#comment-13409517 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
Please note that the guarantees provided must be consistent across api methods exposed - for example, similar reasoning applies to publish.
(1) is not supported in publish iirc.
Currently, only (2) is.
(3) can be inferred if the seq-id is returned : but there is no requirement that not receiving it meant the message was not published (socket lost post delivery or server death, etc) - as in, no transactional guarantees.
Which was the reason (and not to mention minimize hedwig changes :) ), we restricted to (2) - make it inline with rest of api, while providing reasonable assurance of delivery.
Having said that, as long as there is reasonable assurance that best case effort was made to send consume request to server, any additional guarantees would be better (but would have a higher cost, which needs to be factored in - ack from server for example) !
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402165#comment-13402165 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
bq. Also, it would be better to make client related changes to both java and c++ client, to keep consistency between java & c++ clients. if they are too big to be in a single jira, it would be better to have a sub-task to handle c++ changes.
>From my cursory investigation, changes to C++ code is very invasive to support these changes.
Since there was not immediate usecase for it, I have left it as a TODO - something which can be attempted when there is interest in the same.
bq. for responses, it would be PublishResponse not ServerResponse. I think you could leverage the method that I described in the comment of BOOKKEEPER-310.
Agree, it will change to PublishResponse - we can get rid of the ServerResponse client class and rely on protocol's PublishResponse directly.
bq. changing consume behavior should be a separated jira to make things more clearly.
As mentioned elsewhere, since changes are interdependent, they co-exist in the JIRA : if it is becoming a problem to review it can be split.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Assigned] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Flavio Junqueira reassigned BOOKKEEPER-311:
-------------------------------------------
Assignee: Mridul Muralidharan
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424784#comment-13424784 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
Could be added as pre-commit hook to validate coding conventions and reject if it violates it for java/cpp files ...
Ditto for number of warnings, etc.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Description:
Primary changes are :
a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
In addition, there are also fixes for
d) Fix NPE's observed as part of testing JMS provider.
was:
Primary changes are :
a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
In addition, there are also fixes for
d) Fix NPE's observed as part of testing JMS provider.
Summary: Changes in hedwig client api to support JMS spec (was: Protocol changes in hedwig client api to support JMS spec)
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424774#comment-13424774 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
The line length was what was mentioned by Sijie : 80-100 chars per line ...
Since the 80 character limit in java coding std predates modern editors (for a time of tty's/consoles using vi/emacs/printed code), I assumed breaking it was fine.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client-consume.patch.1
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13420607#comment-13420607 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
@Mridul, as Flavio's suggestions, please fold long lines into shorter ones, which makes it consistent with BookKeeper project's code style.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13399162#comment-13399162 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
Please note that there are no changes being made to cpp impl.
The bugs fixed above are not applicable (b, c, d), and the change in functionality (a) does not have any consumers - hence not implemented.
[Not consuming the returned seq-id to publish response should not cause an error imo - please correct me if I am wrong].
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: (was: hedwig-client.patch.3)
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424777#comment-13424777 ]
Flavio Junqueira commented on BOOKKEEPER-311:
---------------------------------------------
One issue, unrelated to this patch, is that we have been just loosely following the code conventions we have set for the project, mainly because they are too strict. It might be a good idea to revisit code conventions and have it in such a way that it makes sense to enforce. I personally don't feel like enforcing 80-character lines.
For this patch, it is ok with me to have it in.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13416121#comment-13416121 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
+1 for the new patch.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Uma Maheswara Rao G (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424780#comment-13424780 ]
Uma Maheswara Rao G commented on BOOKKEEPER-311:
------------------------------------------------
Yes, 80 characters per line will be small in some cases and code may looks ugly if we wrap it to 2 lines. Recently, Hbase also revisited for this point and made their convention to 100+, I remember.
May be, we can have that discussion in the mailing list?
{quote}
is that we have been just loosely following the code conventions we have set for the project, mainly because they are too strict.
{quote}
I am not sure, anyone is violating these conventions. IMO, we have to be very strict towards the conventions for the long term maintenance(neatly) of the code. Otherwise, code will become too ugly with the different conventions placed.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13401983#comment-13401983 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
as I comment in https://issues.apache.org/jira/browse/BOOKKEEPER-310?focusedCommentId=13401940&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13401940, it would be better to have a separated jira to support returning seq id for publish request, instead of putting them in different jiras.
Also, it would be better to make client related changes to both java and c++ client, to keep consistency between java & c++ clients. if they are too big to be in a single jira, it would be better to have a sub-task to handle c++ changes.
> ServerResponse
for responses, it would be PublishResponse not ServerResponse. I think you could leverage the method that I described in the comment of BOOKKEEPER-310.
> consume
changing consume behavior should be a separated jira to make things more clearly.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13420551#comment-13420551 ]
Flavio Junqueira commented on BOOKKEEPER-311:
---------------------------------------------
In general the patch looks good to me, but I have a few minor comments:
- There is one TODO in the patch. For every TODO, it would be best to have a jira created. If the TODO is a blocker, the jira needs to be marked as such.
- This patch seems to introduce a discrepancy between the api of the java and of the C clients. If not fixed in this patch, which I'm fine with no doing, then we need to create a jira a mark the jira as a blocker.
- FORMAT: There are some long lines in this patch, in particular the javadoc for asyncPublishWithResponse.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client.patch.3
Removed all optional changes for base functionality.
The patch now only contains changes for retrieving seq-id of a published message, and nothing else.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409242#comment-13409242 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
for consume request:
1) the changes break backward compatibility. it changed the consume semantic. if existed applications used consume as async callback, it broken existed applications' assumption. one way I think we might not change consume's semantic, adding another call which returns only when consume request is written to the channel. this new call name might be 'syncConsume' or other better name.
2) even we have 'syncConsume' like method to ensure netty wrote the consume request to server. we still can't guarantee the consume request is received and processed by hub server. so adding such 'syncConsume' could not provide any semantic guarantee to the consume behavior.
what kind of semantic for consume request is needed by JMS provider? the request is wrote to the channel? the request is processed by the hub sever?
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client.patch.5
Changed line length to roughly 100 chars or so.
Note that change is limited to what modified in the patch (not to rest of codebase).
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13424896#comment-13424896 ]
Sijie Guo commented on BOOKKEEPER-311:
--------------------------------------
{quote}
IMO, we have to be very strict towards the conventions for the long term maintenance(neatly) of the code. Otherwise, code will become too ugly with the different conventions placed.
{quote}
agreed. +1.
{quote}
Recently, Hbase also revisited for this point and made their convention to 100+, I remember.
{quote}
yes. 100 is a better wide to control the format, because we had lots of callbacks written in both BOOKKEEPER and Hedwig.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4, hedwig-client.patch.5
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client-publish.patch.1
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13420587#comment-13420587 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
Sijie Guo has hopefully answered about the JIRA's
bq. FORMAT: There are some long lines in this patch, in particular the javadoc for asyncPublishWithResponse.
Just to confirm you are referring to the @link tag in javadoc of asyncPublishWithResponse ?
If yes : it was autogenerated, let me try to remove the fully qualified names and see if that works.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Updated] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-311:
-------------------------------------------
Attachment: hedwig-client.patch.4
Based on review comment, add "respBody.hasPublishResponse()" before returning respBody.getPublishResponse()
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch, hedwig-client.patch.3, hedwig-client.patch.4
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13407605#comment-13407605 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
Splitting into
a) consume patch - which deals with changes to consume methods and changes to ensure that consume() message buffering do not get lost on close().
The callback changes from (b) do overlap (partially) with this.
b) Changes to publish method(s) to return the PublishResponse to client. Also includes changes to fix NPE's found while running the testcases.
hedwig-client.patch is no longer relevant.
Note that the protocol changes for (b) are NOT part of the patch since generated code overlaps heavily with metadata changes.
The relevant difference to proto is a minor 5 line change :
{quote}
message RegionSpecificSeqId {
@@ -138,6 +166,18 @@ message PubSubResponse{
optional Message message = 5;
optional bytes topic = 6;
optional bytes subscriberId = 7;
+
+ // the following fields are sent by other requests
+ optional ResponseBody responseBody = 8;
+}
+
+message PublishResponse {
+ // If the request was a publish request, this was the message Id of the published message.
+ required MessageSeqId publishedMsgId = 1;
+}
+
+message ResponseBody {
+ optional PublishResponse publishResponse = 1;
}
{quote}
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409321#comment-13409321 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
I think I messed up numbering (towards the end) of the various problem in my previous comment and possibly making them difficult to comprehend ... not sure how to edit it to clean it up.
In the last two lines, when I refer to Problem (2) and Problem (1), I am referring to the overall problems (not the specific ones I numbered as 1 and 2 later on).
Sorry for the errors.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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
[jira] [Commented] (BOOKKEEPER-311) Changes in hedwig client api to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409319#comment-13409319 ]
Mridul Muralidharan commented on BOOKKEEPER-311:
------------------------------------------------
bq. 1) the changes break backward compatibility. it changed the consume semantic. if existed applications used consume as async callback, it broken existed applications' assumption. one way I think we might not change consume's semantic, adding another call which returns only when consume request is written to the channel. this new call name might be 'syncConsume' or other better name.
When discussing with Ivan, it looked like the existing consume() method was not consistent with rest of the api exposed from the interface all async methods were prefixed with 'async' and the sync ones were not.
We assumed the implementation of consume was an oversight (particularly since it did not have a async variant) - hence the addition of an additional explicit asyncConsume.
Having said that, you are right, it is an intentional interface behavioral change introduced.
Note that there are two aspects to the overall change :
Basically, what happens when user does
* consume() and then close() (no-auto ack mode) vs
* close() (ack mode - controlled via auto_send_consume_message_enabled=true (default) and consumed_messages_buffer_size (default 5) ).
This in context of the fact that there is option of automatic consume acknowledgement, and buffering of these consume acknowledgements implemented in java api. (Actually, you can mix ack-mode with explicit consume() too, and vice versa btw - the latter resulting in some interesting issues).
The changes to client code (on consume related changes) are to handle issues with these two cases.
In the first case :
If consume remains async, there are two possibilities here :
1) consume() results in request being sent to server, and so server does not redeliver the message again.
2) consume() request was in flight (within netty), while close() is executed - resulting in socket close before request makes it out.
As you mentioned, we can ofcourse keep consume() as async (but with no way to track progress via a future) and introduce a syncConsume() - functionally, other than inability to track future, this will be functionally equivalent way to resolve the bug !
Note that, as you mentioned in point 2 above, other than netty 'telling us' about delivery of request to server - there is nothing much else we can do. Practically, this is good enough.
It is a best case effort, and does not give transactional gaurantee's.
For the second case (close() with auto-ack) :
The changes to *ResponseHandler via addition of handleChannelClosedExplicitly (rename ?) is to handle this case.
Ensure that buffered state (in this case, consume'd seq-id which is not yet ack'ed to server) is written before closing socket.
Even this is, ofcourse, best case effort : note that this is used for sending buffered seq-id but could be used for others too (now or in future).
(1) is the desired behavior, and (2) is actually fairly common - please note that this observed and then fixed, not other way around :-) I was really looking to no change hedwig in any way possible.
a) The sync aspect comes in ONLY when the invocation of the method results in a request being sent to the server.
b) If consume() does not result in request to server (due to buffering of consume requests), then changes to close() handle the delivery of buffered seq'id.
To recap:
The issue we are trying to resolve is, if message is consume'd by user, within reasonable gaurantee's, it must not be sent back to him.
Problem (2) above meant that upto last 4 messages might always be sent back to client.
Problem (1) meant that consume + close might send last message (or last N if batch consume by user !) back to him.
> Changes in hedwig client api to support JMS spec
> ------------------------------------------------
>
> Key: BOOKKEEPER-311
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-311
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-consume.patch.1, hedwig-client-publish.patch.1, hedwig-client.patch
>
>
> Primary changes are :
> a) Add support for returning seq-id for a publish request. This is an api change (backwardly compatible for users).
> b) Make consume a sync consume, with addition of an asyncConsume - this is to ensure that invoking consume() ensure request makes to server before returning (with what reasonable gaurantees that netty allows).
> c) Ensure that explicit close'ing of session will flush buffered consume seq-id's when auto-ack is enabled (default in hedwig java client).
> In addition, there are also fixes for
> d) Fix NPE's observed as part of testing JMS provider.
--
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