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 22:51:43 UTC
[jira] [Created] (BOOKKEEPER-309) Protocol changes in hedwig to
support message properties, and make body optional
Mridul Muralidharan created BOOKKEEPER-309:
----------------------------------------------
Summary: Protocol changes in hedwig to support message properties, and make body optional
Key: BOOKKEEPER-309
URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
Project: Bookkeeper
Issue Type: Sub-task
Reporter: Mridul Muralidharan
JMS spec compliance requires three changes to the protocol.
a) Support for message properties.
b) Make body optional (message contains only properties).
c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Flavio Junqueira reassigned BOOKKEEPER-309:
-------------------------------------------
Assignee: Mridul Muralidharan
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Assignee: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13417992#comment-13417992 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
@Flavio, since it is protobuf generated code, I would prefer not to do any changes to such generated code.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402706#comment-13402706 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
{quote}
But JMS assumes some basic functionality to be provided by every messaging system : the reason for changes to hedwig protocol was necessitated due to lack of these in hedwig : a) lack of support for metadata for messages, b) returning published message's id.
The list is actually much longer, but as I mentioned elsewhere, we simulate a lot of the changes within provider (some of them poorly as a kludge) to minimize changes to hedwig and protocol : which was a priotity one requirement.
{quote}
b) is a dependency for you to implement JMS provider. so my suggestion when reviewing all sub tasks is you should put all changes related to b) together in a separated jira like 'support returning published message id'. Not spread it over several jiras mixed with other features.
besides that, I found you change 'consume' behavior. so 'changing consume behavior' would be another jira to handle it, not mix it with other jiras.
so for BOOKKEEPER-308, you have several tasks need to be done:
1. Support returning published message id
2. Changing consume behavior
3. Add metadata support in Message for JMS
4. JMS provider implementation
1, 2, 3 are independent, while 4 depends on 1, 2, 3. How about changing sub tasks as above? so we don't need to spread discussion over different jiras, make each jira focus on one feature.
{quote}
In my opinion, relying on raw bytes to convey semantics for metadata will make interoperable implementations impossible as features evolve, and/or as more language bindings are added; so when taking a call, please take that into consideration.
{quote}
I don't think there is language binding issue for a generic key/bytes header, because you are using protobuf. You could create a kind of message called JMSValue.
{code}
message JMSValue {
enum Type {
BOOLEAN = 1;
BYTE = 2;
SHORT = 3;
INT = 4;
LONG = 5;
FLOAT = 6;
DOUBLE = 7;
STRING = 8;
};
required Type type = 1;
optional bool booleanValue = 2;
optional sint32 byteValue = 3;
...
}
{code}
What I concerned most is that mixing hedwig protocol with jms protocol together is not a good idea. It introduced overhead for those native hedwig client users. I think it would not be difficult to decouple them.
{code}
message JMSMessage {
+ repeated Key2Boolean booleanMetadata = 4;
+ repeated Key2Byte byteMetadata = 5;
+ repeated Key2Short shortMetadata = 6;
+ repeated Key2Integer integerMetadata = 7;
+ repeated Key2Long longMetadata = 8;
+ repeated Key2Float floatMetadata = 9;
+ repeated Key2Double doubleMetadata = 10;
+ repeated Key2String stringMetadata = 11;
+ optional JmsBodyType jmsBodyType = 12;
+}
+
+enum JmsBodyType {
+ STREAM = 0;
+ MAP = 1;
+ TEXT = 2;
+ OBJECT = 3;
+ BYTES = 4;
+ // no payload, only message.
+ MESSAGE = 5;
+}
+
+message Key2Boolean {
+ required string key = 1;
+ required bool value = 2;
+}
+
+message Key2Byte {
+ required string key = 1;
+ // since protobuffer does not support sint8, we use sint32. We need this explicitly to differentiate between
+ // byte and int headers.
+ required sint32 value = 2;
+}
+
+message Key2Short {
+ required string key = 1;
+ // since protobuffer does not support sint16, we use sint32. We need this explicitly to differentiate between
+ // byte and int headers.
+ required sint32 value = 2;
+}
+
+message Key2Integer {
+ required string key = 1;
+ required sint32 value = 2;
+}
+
+message Key2Long {
+ required string key = 1;
+ required sint64 value = 2;
+}
+
+message Key2Float {
+ required string key = 1;
+ required float value = 2;
+}
+
+message Key2Double {
+ required string key = 1;
+ required double value = 2;
+}
+
+message Key2String {
+ required string key = 1;
+ required string value = 2;
}
{code}
you could have a JMSMessage protobuf definition to carry all jms related things, and put it in JMS proto file which is just used in hedwig-jms-client. And a JMS
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13400685#comment-13400685 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
{quote} I would prefer not to put anything JMS specific into the hedwig protocol. If JMS is running on top, of hedwig, it should run on top. There shouldn't be cross contamination between the protocols. I think this is quite easy though.
{quote}
There were two (somewhat) conflicting requirements :
a) to be able to interoperate seemlessly (as much as possible) with existing hedwig clients - that is, mixing producers and consumers between "native" hedwig api and jms provider clients.
b) to be able to use jms specific features without breaking backward compatibility - so that producer or consumer which is using jms specific idioms can continue to work without breaking other clients.
Given this, the options to support message metadata were :
1) make it inline as part of the message itself : this will break all existing clients since they will not be able to parse the content of the message.
2) Extend protocol to add support for metadata : so that only clients which need to query, would do so, while the rest can continue to ignore the additional metadata fields.
My understanding of protobuffer is very naive at best, but I was given to understand that this sort of evolution was supported : please do correct me if I am wrong.
Given this, I choose the approach (2).
{quote}
JmsBodyType is an enum. It can be very easily implemented as a byte in the metadata, and a byte->enum mapping used on the jms client.
{quote}
You are right, this is a JMS specific metadata change - something which slipped my mind when I was making the changes.
The only constraint is, we have to ensure it is a global mapping - so that different interpretations dont arise : which is why I used an enum to document/enforce it in protocol itself.
If there is another registry in client api (or elsewhere) where this can be specified, it will probably be a more extensible approach (to reduce protocol changes).
{quote}
I think there should be only 1 metadata in Message, which is a repeated of a generic 'KeyValue' type.
...
In fact, I'm inclined to think there should only be a 'bytes' value type, and leave it up to the application to decode it correctly. Sijie was talking about adding metadata recently, so we should also discuss this with him.
{quote}
I like the generic KeyValue metadata - it was something I did not pursue in detail since I was not sure what all value types will be required.
In JMS, it is restricted to primitives, but in hedwig will it include other data structures, etc ?
I was hoping the review process will thrash out a better metadata format based on the collective experience in use of hedwig - just that it be something which will allow the basic requirements to be supported (in addition to others) !
About leaving it as 'bytes' - I would prefer if we had the value cleanly specified : conflicting interpretations between clients can be avoided - ideally, if there was some sort of union support in protobuf, all the more the better; else, the proposal above sounds fine (with extensions as required).
{quote}
body cannot be changed from required to optional unfortunately, as this is a BC break. However, we can have a 'bodyEmpty' boolean metadata value, which the client can interpret to mean the same thing.
{quote}
IIRC you had mentioned this before and it slipped my mind, my apologies !
I will add an optional flag and revert changes to make body optional. In case body is missing, I will set it to an new byte[0] - hope that would not break things ...
{quote}
1 final proceedure point. When you do a git diff, use (--no-prefix). It means that a patch will apply with patch -p0, which is a convention we prefer (not that it's particularly better than -p1, but its like driving on one side of the road).
{quote}
Will do !
Do you want me to regenerate all patches attached to the issue(s) ?
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13417641#comment-13417641 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
@Flavio
I assume you are referring to protobuf generated code ?
Is there something specific we can do to get protobuf to generate code conforming to the conventions ?
Or am I missing something else ?
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409332#comment-13409332 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
bq. 1) first is to encode jms message in the body of hedwig message. and mark the hedwig message type as 'JMS' in messageType field in MessageHeader.
If I understand this correctly, it will mean that it will not be possible for publisher's using jms provider to interact with hedwig clients (and vice versa) right ?
As in, clients which depends only on body will now see body plus metadata encoded within ?
If that is the case, then I dont think we should pursue it ...
Or did I miss something ?
bq. 2) second is to leverage hedwig generic properties field in MessageHeader. you could have JmsValue type in jms.protocol in hedwig-jms-client package as below:
If I am not wrong, I think Ivan proposed similar approach, though you give much more details above ?
My only concern with this is that
* It does not allow for interoperability for and via message headers.
* When message headers stabilize further in hedwig, particularly for server side filtering, etc; we will need to reimplement this piece in provider.
If it is just a metter of more time to thrash out the generic approach to how typed metadata will be handled in hedwig messages, we can hold off on this JIRA until that is resolved !
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Attachment: hedwig-protocol.patch.5
Regenerated with 2.4.1 protobuffer
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Attachment: hedwig-protocol.patch.3
Changes to protocol to return seq-id as part of publish response as per review comments.
Also includes protocol change from BOOKKEEPER-78 for defining message headers.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402158#comment-13402158 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
>From what I understand, stomp is a wire level protocol - JMS is not, it is expected to expose existing messaging system functionality in a vendor neutral manner.
But JMS assumes some basic functionality to be provided by every messaging system : the reason for changes to hedwig protocol was necessitated due to lack of these in hedwig : a) lack of support for metadata for messages, b) returning published message's id.
The list is actually much longer, but as I mentioned elsewhere, we simulate a lot of the changes within provider (some of them poorly as a kludge) to minimize changes to hedwig and protocol : which was a priotity one requirement.
Other than the subset above which is bare minimum and cant be simulated (published message's id) or cant be done in an interoperable manner (metadata for messages); rest are simulated in the provider (filtering, nolocal, transactions, etc).
In my opinion, relying on raw bytes to convey semantics for metadata will make interoperable implementations impossible as features evolve, and/or as more language bindings are added; so when taking a call, please take that into consideration. I would prefer if the metadata changes are generic enough that it satisfies not just these subset of requirements, but also additional metadata requirements that might exist to minimize future need to change protocol.
Since I do not have enough visibility into this, I will leave it to the domain experts.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Attachment: hedwig-protocol.patch
Protocol changes which implement the changes mentioned in description
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409430#comment-13409430 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
{quote}
Or did I miss something ?
{quote}
that's why I suggested you to split BOOKKEEPER-308 by features. so we could focus discussion on specified feature at a single jira instead of spreading it over different jiras.
{quote}
If I understand this correctly, it will mean that it will not be possible for publisher's using jms provider to interact with hedwig clients (and vice versa) right ?
As in, clients which depends only on body will now see body plus metadata encoded within ?
If that is the case, then I dont think we should pursue it ...
{quote}
as I described on BOOKKEEPER-78 ( https://issues.apache.org/jira/browse/BOOKKEEPER-78?focusedCommentId=13403094&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13403094 ), different protocols could leverage a system property 'messageType' in MessageHeader to know how to encode/decode the data according to its protocol. I think it could resolve interoperability issue you care about.
{quote}
It does not allow for interoperability for and via message headers.
{quote}
I think what you need is tell the messages published from different kind of publishers. I think 'messageType' in MessageHeader as proposed in BOOKKEEPER-78 could resolve your concern.
{quote}
When message headers stabilize further in hedwig, particularly for server side filtering, etc; we will need to reimplement this piece in provider.
{quote}
I don't think you need to do lots of work. From a generic view, a message filter would answer true or false when giving a Message. for you client-side filter now, you could get header from the message, do filtering logic, and return true or false. server-side fileter just enable you run it in server-side without doing nothing.
actually we just finished the message filter work and I would attach the patches these two days to let you see whether it makes sense for you or not.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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] [Comment Edited] (BOOKKEEPER-309) Protocol changes in hedwig
to support JMS spec
Posted by "Ivan Kelly (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13400492#comment-13400492 ]
Ivan Kelly edited comment on BOOKKEEPER-309 at 6/25/12 2:04 PM:
----------------------------------------------------------------
I would prefer not to put anything JMS specific into the hedwig protocol. If JMS is running on top, of hedwig, it should run on top. There shouldn't be cross contamination between the protocols. I think this is quite easy though.
JmsBodyType is an enum. It can be very easily implemented as a byte in the metadata, and a byte->enum mapping used on the jms client.
I think there should be only 1 metadata in Message, which is a repeated of a generic 'KeyValue' type.
{code}
message KeyValue {
enum ValueType {
BOOLEAN = 1;
BYTE = 2;
SHORT = 3;
INT = 4;
LONG = 5;
FLOAT = 6;
DOUBLE = 7;
STRING = 8;
};
required ValueType type = 1;
required string key = 2;
optional bool booleanValue = 3;
optional sint32 byteValue = 4;
...
}
{code}
In fact, I'm inclined to think there should only be a 'bytes' value type, and leave it up to the application to decode it correctly. Sijie was talking about adding metadata recently, so we should also discuss this with him.
body cannot be changed from required to optional unfortunately, as this is a BC break. However, we can have a 'bodyEmpty' boolean metadata value, which the client can interpret to mean the same thing.
1 final proceedure point. When you do a git diff, use (--no-prefix). It means that a patch will apply with patch -p0, which is a convention we prefer (not that it's particularly better than -p1, but its like driving on one side of the road).
was (Author: ikelly):
I would prefer not to put anything JMS specific into the hedwig protocol. If JMS is running on top, of hedwig, it should run on top. There shouldn't be cross contamination between the protocols. I think this is quite easy though.
JmsBodyType is an enum. It can be very easily implemented as a byte in the metadata, and a byte->enum mapping used on the jms client.
I think there should be only 1 metadata in Message, which is a repeated of a generic 'KeyValue' type.
{code}
message KeyValue {
enum ValueType {
BOOLEAN = 1;
BYTE = 2;
SHORT = 3;
INT = 4;
LONG = 5;
FLOAT = 6;
DOUBLE = 7;
STRING = 8;
};
required ValueType type = 1;
required string key = 2;
optional bool booleanValue = 3;
optional sint32 byteValue = 4;
...
}
In fact, I'm inclined to think there should only be a 'bytes' value type, and leave it up to the application to decode it correctly. Sijie was talking about adding metadata recently, so we should also discuss this with him.
body cannot be changed from required to optional unfortunately, as this is a BC break. However, we can have a 'bodyEmpty' boolean metadata value, which the client can interpret to mean the same thing.
1 final proceedure point. When you do a git diff, use (--no-prefix). It means that a patch will apply with patch -p0, which is a convention we prefer (not that it's particularly better than -p1, but its like driving on one side of the road).
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13417796#comment-13417796 ]
Flavio Junqueira commented on BOOKKEEPER-309:
---------------------------------------------
I was thinking about something like an eclipse format over the files in the patch, but that might generate undesirable changes, I'm not sure.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13407657#comment-13407657 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
> Protocol changes for adding metadata.
How is the proposal to add MessageHeader I proposed in BOOKKEEPER-78?
> The diffs also overlap with client publish changes.
Since the protocol changes for client publish is introduced for BOOKKEEPER-310 and BOOKKEEPER-311, why not put them together? Because the publish you changed could only work after BOOKKEEPER-309, BOOKKEEPER-310 and BOOKKEEPER-311 are in.
As my previous comment, I suggested you split you sub-tasks by features, each feature is done as a jira. so if a jira is committed, the feature is available. following this practice, BOOKKEEPER-308 could be separated by several features.
1) message header : include protobuf changes for message header.
2) publish changes : include protobuf changes for publish response, hedwig client & server side changes to support returning message seq id for publish request.
3) consume changes
4) jms provider.
so each feature could be a jira. since there is protobuf changes in both 1) and 2), you could generate the patch of 2) based on 1).
does it make sense for you?
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409258#comment-13409258 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
@Mridul, thanks for explanation. comments are as blow:
{quote}
Without type information, any attempts to filtering will be broken.
More specifically, in case of JMS, there are strict rules in terms of conversion between types - which cant be inferred without the type.
{quote}
for resolving jms type, there are two kind of solutions:
1) first is to encode jms message in the body of hedwig message. and mark the hedwig message type as 'JMS' in messageType field in MessageHeader.
2) second is to leverage hedwig generic properties field in MessageHeader. you could have JmsValue type in jms.protocol in hedwig-jms-client package as below:
{code}
+message JmsValue {
+ enum ValueType {
+ BOOLEAN = 1;
+ BYTE = 2;
+ SHORT = 3;
+ INT = 4;
+ LONG = 5;
+ FLOAT = 6;
+ DOUBLE = 7;
+ STRING = 8;
+ // raw bytes. (custom correlation id, for example, uses this : though we dont support it right now).
+ BYTES = 9;
+ };
+
+ required ValueType type = 1;
+
+ optional bool booleanValue = 3;
+ optional sint32 byteValue = 4;
+ optional sint32 shortValue = 5;
+ optional sint32 intValue = 6;
+ optional sint64 longValue = 7;
+ optional float floatValue = 8;
+ optional double doubleValue = 9;
+ optional string stringValue = 10;
}
{code}
you could serialize JmsValue as a byte array and put in hedwig's message header and you also could deserialize bytes to a JmsValue.
both solutions could let jms protocol in its package, so its changes just take effects on hedwig-client-jms module not other modules.
{quote}
Having said that, the individual diff's currently in the jira's loosely follow this already - (2) and (3) are mostly split along consume and publish whereever possible, but are not independently applicable due to hedwig code level overlap.
{quote}
yes. the new patches are quite loosely coupled. I had some comments on them on BOOKKEEPER-310, BOOKKEEPER-311.
separating the patches by features is not only easy for reviewing but also for discussion when there are different opinions. as my comments on message header and consume request (in BOOKKEEPER-311), there are still different opinions so it would better to have separated jiras for discussion.
for supporting returning message seq id, we both agreed on how to resolve it and you had did a great work on it, so I think it could go to be committed first and I created a sub-task BOOKKEEPER-331 for it. It might be great to put the related changes from BOOKKEEPER-309, BOOKKEEPER-310, BOOKKEEPER-311 together when you update the patch.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13414979#comment-13414979 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
oh damn, forgot about protoc version ... sigh.
Let me regenerate this patch with diffs based on 2.4.1 - thanks !
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409504#comment-13409504 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
bq. as I described on BOOKKEEPER-78 ( https://issues.apache.org/jira/browse/BOOKKEEPER-78?focusedCommentId=13403094&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13403094 ), different protocols could leverage a system property 'messageType' in MessageHeader to know how to encode/decode the data according to its protocol. I think it could resolve interoperability issue you care about.
I am not sure if I am conveying what I mean by interoperability adequately : but I will give one more shot.
Consider HTTP for example - a firewall or proxy does not need to understand anything about what is contained within the http response body to make decisions about it : it does so via the status and headers. Similarly a lot of clients which consume http.
Those two aspects form the basis for interoperability between diverse users of http protocol (which also describes what the body is btw).
The proposal in hedwig right now require a wire-level understanding of how the message header is encoded : with different clients doing so differently.
headers can be the generic means for different - potentially incompatible clients - to interact in an interoperable manner. Think OCP.
Filters are an example of it - specify a filter which says deliver only messages with a specific set of constraints satisfied (including user specific headers) - in a decoupled manner : with different clients requiring different ways of interpreting what the constraints for it are.
Please note that this is orthogonal to using different topic's for different usecases.
The SQL-like syntax supported in JMS spec for filtering also follows similar rationale :
Have typed headers, define how to filter them using SQL, and rely on provider/server to deliver only messages that a client can actually use/interpret/needs from the Topic(s) subscribed to.
message type is just one way (though crucial) way in which clients interoperate (jmsBodyType iirc), but there are others and potentially user specific means to do so.
If hedwig does not require this sort of extensibility, then ofcourse the discussion is moot : and it makes logical sense to go with application level headers.
bq. actually we just finished the message filter work and I would attach the patches these two days to let you see whether it makes sense for you or not.
Great ! I will watch that issue for updates. Thanks !
Greate !
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Attachment: hedwig-protocol.patch.1
Protocol changes for adding metadata.
It is standalone (except for hedwig-jms-client depending on it).
The diffs also overlap with client publish changes - it is not possible to separate them out imo due to the generated code.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402907#comment-13402907 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
{quote}
so for BOOKKEEPER-308, you have several tasks need to be done:
1. Support returning published message id
2. Changing consume behavior
3. Add metadata support in Message for JMS
4. JMS provider implementation
1, 2, 3 are independent, while 4 depends on 1, 2, 3. How about changing sub tasks as above? so we don't need to spread discussion over different jiras, make each jira focus on one feature.
{quote}
To clarify the list above, primarily : changes to 1 are spread between client, server and protocol; changes for 2 is local to client; changes to 3 is local to protocol; ofcourse 4 depends on all of the above implicitly for functionality.
Unfortunately, changes to client for 1 and 2 are interdependent (getCallback(), etc) - 2 was not an expected change, had to be introduced due to failing tests much later in the implementation and so depends on 1 implicitly (I did not know consume() was async inspite of name !).
I think more time and effort is being expended to clarify why things are interdependent; I might be better off simply splitting the patches if reviewing them is causing problems :-) I will get to it later this week when I have some time.
Regarding having a bytes header type -
The questions would be :
* Is there a need for generic metadata in hedwig ? If yes, then we have to do it such that the metadata types supported in JMS are part of it.
* What sort of interoperability is being considered between various producers and consumers - our assumption was there must be as much as possible (except for domain specific things like Serializable message type, etc).
Note that metadata in JMS is primarily required for two reasons :
* headers added by JMS provider based on message producer's & consumer's expectation from JMS (filtering, type info, simulating expiration, etc).
* headers added by message producer's explicitly for consumption at the client's (filtering, user specific, etc).
Note that one or more producer(s) or the consumser(s) need not necessarily be using JMS : which is the reason for trying to keep metadata in protocol itself to allow for interoperability.
If this usecase does not make sense, then yes, we can have a JMS provider specific private interpretation of a bytes header - with impact on future evolution and interoperability.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13418123#comment-13418123 ]
Flavio Junqueira commented on BOOKKEEPER-309:
---------------------------------------------
sounds fine. +1.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13407756#comment-13407756 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
.bq How is the proposal to add MessageHeader I proposed in BOOKKEEPER-78?
Without type information, any attempts to filtering will be broken.
More specifically, in case of JMS, there are strict rules in terms of conversion between types - which cant be inferred without the type.
.bq regarding splitting into different jira's.
As I explained before, and the reason I grouped the jira's as tasks under a single umbrella JIRA, is because the tasks are unfortunately interdependent.
I do understand it makes reviewing them a bit of a pain, and the inability to apply them separately - but they cant be split and applied independently (except for server changes I guess) due to overlap.
Unfortunately, I do not have the cycles to try to create additional 4 partial workspaces for this : which wont be functionally complete in case of (2) and (3), or untestable - in case of only (1); and unusable/broken - in case of only (4).
Having said that, the individual diff's currently in the jira's loosely follow this already - (2) and (3) are mostly split along consume and publish whereever possible, but are not independently applicable due to hedwig code level overlap.
Hope you understand.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Flavio Junqueira (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13417196#comment-13417196 ]
Flavio Junqueira commented on BOOKKEEPER-309:
---------------------------------------------
I only have a small comment with respect to format here. The generated code seems to be generating very long lines, and it would be great if we could adhere to the code conventions:
https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
Otherwise, lgtm +1.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13401931#comment-13401931 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
I don't like the idea to put JMS related spec in hedwig protocol. from my understanding, JMS would be a kind of protocol sits on top of Hedwig, so a JMS message would be the body of hedwig message.
I'd prefer to make the hedwig protocol independent from other protocols (like JMS, stomp). So if user want to use JMS, it could use hedwig-jms-client; if user want to use stomp protocol, there might be a separated module hedwig-stomp-client working on stomp protocol.
{quote}
In fact, I'm inclined to think there should only be a 'bytes' value type, and leave it up to the application to decode it correctly. Sijie was talking about adding metadata recently, so we should also discuss this with him.
{quote}
Yes. I agreed Ivan's idea. Adding a generic header (properties: string key, bytes value) part of Hedwig Message would be a good idea. so different protocols sits on top of hedwig could leverage the header part to store its customized data, which might be used to filter message without looking into message body.
FYI: Stomp protocol : http://stomp.github.com//stomp-specification-1.1.html
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Summary: Protocol changes in hedwig to support JMS spec (was: Protocol changes in hedwig to support message properties, and make body optional)
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Ivan Kelly (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13400492#comment-13400492 ]
Ivan Kelly commented on BOOKKEEPER-309:
---------------------------------------
I would prefer not to put anything JMS specific into the hedwig protocol. If JMS is running on top, of hedwig, it should run on top. There shouldn't be cross contamination between the protocols. I think this is quite easy though.
JmsBodyType is an enum. It can be very easily implemented as a byte in the metadata, and a byte->enum mapping used on the jms client.
I think there should be only 1 metadata in Message, which is a repeated of a generic 'KeyValue' type.
{code}
message KeyValue {
enum ValueType {
BOOLEAN = 1;
BYTE = 2;
SHORT = 3;
INT = 4;
LONG = 5;
FLOAT = 6;
DOUBLE = 7;
STRING = 8;
};
required ValueType type = 1;
required string key = 2;
optional bool booleanValue = 3;
optional sint32 byteValue = 4;
...
}
In fact, I'm inclined to think there should only be a 'bytes' value type, and leave it up to the application to decode it correctly. Sijie was talking about adding metadata recently, so we should also discuss this with him.
body cannot be changed from required to optional unfortunately, as this is a BC break. However, we can have a 'bodyEmpty' boolean metadata value, which the client can interpret to mean the same thing.
1 final proceedure point. When you do a git diff, use (--no-prefix). It means that a patch will apply with patch -p0, which is a convention we prefer (not that it's particularly better than -p1, but its like driving on one side of the road).
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13414890#comment-13414890 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
@Mridul, I think you need to use protoc2.4.1 to generate the code instead of using protoc2.3.0. please check the version of protoc you used to generate the code first.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402937#comment-13402937 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
{quote}
I might be better off simply splitting the patches if reviewing them is causing problems
{quote}
Splitting tasks by features is better to track dependencies and also good for reviewing.
{quote}
Is there a need for generic metadata in hedwig ? If yes, then we have to do it such that the metadata types supported in JMS are part of it.
{quote}
Having a generic header in Hedwig Message is necessary for both BOOKKEEPER-321 and BOOKKEEPER-308. I found that there is a jira BOOKKEEPER-78. we could discuss on it to reach an agreement what kind of message header adding for hedwig message.
{quote}
What sort of interoperability is being considered between various producers and consumers - our assumption was there must be as much as possible (except for domain specific things like Serializable message type, etc).
{quote}
It is a good point. Allowing interoperability is quite complex for a system supporting different protocols (for this jira, we have native hedwig protocol and jms protocol). It is an application-dependent work to avoid and resolve. One possible work could be done in hedwig is adding message type in header of hedwig message to let different protocol provider know what type of message it received.
Let me summary my idea of Message Header on BOOKKEEPER-78. so we could have more discussion and try to reach agreement on it.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13416120#comment-13416120 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
+1 for the new patch.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4, hedwig-protocol.patch.5
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13402708#comment-13402708 ]
Sijie Guo commented on BOOKKEEPER-309:
--------------------------------------
{quote}
But JMS assumes some basic functionality to be provided by every messaging system : the reason for changes to hedwig protocol was necessitated due to lack of these in hedwig : a) lack of support for metadata for messages, b) returning published message's id.
The list is actually much longer, but as I mentioned elsewhere, we simulate a lot of the changes within provider (some of them poorly as a kludge) to minimize changes to hedwig and protocol : which was a priotity one requirement.
{quote}
b) is a dependency for you to implement JMS provider. so my suggestion when reviewing all sub tasks is you should put all changes related to b) together in a separated jira like 'support returning published message id'. Not spread it over several jiras mixed with other features.
besides that, I found you change 'consume' behavior. so 'changing consume behavior' would be another jira to handle it, not mix it with other jiras.
so for BOOKKEEPER-308, you have several tasks need to be done:
1. Support returning published message id
2. Changing consume behavior
3. Add metadata support in Message for JMS
4. JMS provider implementation
1, 2, 3 are independent, while 4 depends on 1, 2, 3. How about changing sub tasks as above? so we don't need to spread discussion over different jiras, make each jira focus on one feature.
{quote}
In my opinion, relying on raw bytes to convey semantics for metadata will make interoperable implementations impossible as features evolve, and/or as more language bindings are added; so when taking a call, please take that into consideration.
{quote}
I don't think there is language binding issue for a generic key/bytes header, because you are using protobuf. You could create a kind of message called JMSValue.
{code}
message JMSValue {
enum Type {
BOOLEAN = 1;
BYTE = 2;
SHORT = 3;
INT = 4;
LONG = 5;
FLOAT = 6;
DOUBLE = 7;
STRING = 8;
};
required Type type = 1;
optional bool booleanValue = 2;
optional sint32 byteValue = 3;
...
}
{code}
What I concerned most is that mixing hedwig protocol with jms protocol together is not a good idea. It introduced overhead for those native hedwig client users. I think it would not be difficult to decouple them.
{code}
message JMSMessage {
+ repeated Key2Boolean booleanMetadata = 4;
+ repeated Key2Byte byteMetadata = 5;
+ repeated Key2Short shortMetadata = 6;
+ repeated Key2Integer integerMetadata = 7;
+ repeated Key2Long longMetadata = 8;
+ repeated Key2Float floatMetadata = 9;
+ repeated Key2Double doubleMetadata = 10;
+ repeated Key2String stringMetadata = 11;
+ optional JmsBodyType jmsBodyType = 12;
+}
+
+enum JmsBodyType {
+ STREAM = 0;
+ MAP = 1;
+ TEXT = 2;
+ OBJECT = 3;
+ BYTES = 4;
+ // no payload, only message.
+ MESSAGE = 5;
+}
+
+message Key2Boolean {
+ required string key = 1;
+ required bool value = 2;
+}
+
+message Key2Byte {
+ required string key = 1;
+ // since protobuffer does not support sint8, we use sint32. We need this explicitly to differentiate between
+ // byte and int headers.
+ required sint32 value = 2;
+}
+
+message Key2Short {
+ required string key = 1;
+ // since protobuffer does not support sint16, we use sint32. We need this explicitly to differentiate between
+ // byte and int headers.
+ required sint32 value = 2;
+}
+
+message Key2Integer {
+ required string key = 1;
+ required sint32 value = 2;
+}
+
+message Key2Long {
+ required string key = 1;
+ required sint64 value = 2;
+}
+
+message Key2Float {
+ required string key = 1;
+ required float value = 2;
+}
+
+message Key2Double {
+ required string key = 1;
+ required double value = 2;
+}
+
+message Key2String {
+ required string key = 1;
+ required string value = 2;
}
{code}
you could have a JMSMessage protobuf definition to carry all jms related things, and put it in JMS proto file which is just used in hedwig-jms-client. And a JMSMessage is deserialized as the body of HedwigMessage.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13414752#comment-13414752 ]
Mridul Muralidharan commented on BOOKKEEPER-309:
------------------------------------------------
Also, had to modify protobuf-java back from 2.4.1 to 2.3.0.
I was unable to compile with 2.4.1 inspite of using protobuf lifecycle - the generated code gave compile errors ...
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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-309) Protocol changes in hedwig to
support JMS spec
Posted by "Mridul Muralidharan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-309?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mridul Muralidharan updated BOOKKEEPER-309:
-------------------------------------------
Attachment: hedwig-protocol.patch.4
Incorporate review comments - specifically :
a) Move to latest apache bookkeeper trunk rev
b) remove change(s) from BOOKKEEPER-78 - the rest of the patch DEPENDS on this jira patch as of now to compile and run.
> Protocol changes in hedwig to support JMS spec
> ----------------------------------------------
>
> Key: BOOKKEEPER-309
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-309
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-protocol.patch, hedwig-protocol.patch.1, hedwig-protocol.patch.3, hedwig-protocol.patch.4
>
>
> JMS spec compliance requires three changes to the protocol.
> a) Support for message properties.
> b) Make body optional (message contains only properties).
> c) Return the published message's seq-id in the response.
--
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