Qpid
  1. Qpid
  2. QPID-3720

[Java Broker] Implement Message Grouping

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.15
    • Component/s: Java Broker
    • Labels:
      None

      Description

      Implement message grouping on the Java Broker (similar in style to that implemented in the C++ broker in QPID-3346)

      The Java Broker supports two implementations of message grouping; an implementation with the same semantics as defined by the C++ broker, and an alternative implementation.

      In contrast to the C++ implementation the alternative implementation has the following properties:

      1) Once a group has been assigned to a subscription, assign all further messages from the same group to the same group (whether or not all prior messages in the group have been acknowledged)
      2) The absence of a value in the designated header field for grouping as treated as indicative of a lack of desire for the message to be grouped. Messages with such a lack of a value will be distributed to any available consumer.

      Note that in general 1) is stricter than the condition that is enforced by the C++ broker - however in the case where a client cancels a subscription without first releasing/acknowledging all outstanding messages that have been delivered then you can get "out of order".

      In general the Java implementation is more liberal than the C++ implementation in that for both strategies available in the Java Broker in that

      a) Message grouping is allowed on priority queues, Conflation Queues (and Sorted Queues). (However it has no effect on browsers... which means if you using a ConflationQueue as an LVQ by browsing it you cannot use groups).
      b) The broker allows for non string values for the group header

      In order to choose the alternative strategy (rather than the C++ style grouping semantics) the qpid.shared_msg_group argument should not be set on queue construction (whether grouping is enforced or not depends on whether the qpid.group_header_key argument is non-null), if qpid.shared_msg_group is set to "1" the C++ style grouping is used, otherwise the alternative implementation is used.

        Issue Links

          Activity

          Hide
          Robbie Gemmell added a comment -

          The feature implementation itself seems fine, but the tests could use a couple of changes.

          The actual message recieved should be verified to ensure the correct ordering, as the message might be from the correct group but not be the expected message. Eg in testConsumerCloseGroupAssignment, the test would pass if consumer 2 got message 1, 3 or 4 in any order, but there is an expectation of particular messages and ordering so it should be verified. Also in the tests the group verification should have the expected group as the first of the two values instead of the second.

          Show
          Robbie Gemmell added a comment - The feature implementation itself seems fine, but the tests could use a couple of changes. The actual message recieved should be verified to ensure the correct ordering, as the message might be from the correct group but not be the expected message. Eg in testConsumerCloseGroupAssignment, the test would pass if consumer 2 got message 1, 3 or 4 in any order, but there is an expectation of particular messages and ordering so it should be verified. Also in the tests the group verification should have the expected group as the first of the two values instead of the second.
          Hide
          Robbie Gemmell added a comment -

          Following the addition of the C++ broker style groups (possibly worth noting in the JIRA description? ) I see what looks like an issue with the 'default group' check in the SimpleAMQQueue constructor:

                          String defaultGroup = String.valueOf(arguments.get(QPID_DEFAULT_MESSAGE_GROUP));
                          _messageGroupManager =
                                  new DefinedGroupMessageGroupManager(String.valueOf(arguments.get(QPID_GROUP_HEADER_KEY)),
                                          defaultGroup == null ? QPID_NO_GROUP : defaultGroup,
                                          this);
          

          The check for defaultGroup == null might never be true, because String.valueof() doesnt return null (except possibly in a case the Javadoc isnt that clear on, when it is provided with a non-null Object which has a toString() implementation which returns null ?) and instead gives the String value "null" when provided with a null reference.

          Show
          Robbie Gemmell added a comment - Following the addition of the C++ broker style groups (possibly worth noting in the JIRA description? ) I see what looks like an issue with the 'default group' check in the SimpleAMQQueue constructor: String defaultGroup = String .valueOf(arguments.get(QPID_DEFAULT_MESSAGE_GROUP)); _messageGroupManager = new DefinedGroupMessageGroupManager( String .valueOf(arguments.get(QPID_GROUP_HEADER_KEY)), defaultGroup == null ? QPID_NO_GROUP : defaultGroup, this ); The check for defaultGroup == null might never be true, because String.valueof() doesnt return null (except possibly in a case the Javadoc isnt that clear on, when it is provided with a non-null Object which has a toString() implementation which returns null ?) and instead gives the String value "null" when provided with a null reference.
          Hide
          Rob Godfrey added a comment -

          Addressed the comment by Robbie - assignment of the default group should now work

          Show
          Rob Godfrey added a comment - Addressed the comment by Robbie - assignment of the default group should now work
          Hide
          Robbie Gemmell added a comment -

          Looks good. Updating the JIRA description and/or adding some docs would seem useful before closing out.

          Robbie.

          Show
          Robbie Gemmell added a comment - Looks good. Updating the JIRA description and/or adding some docs would seem useful before closing out. Robbie.
          Hide
          Robbie Gemmell added a comment -

          Closing out

          Show
          Robbie Gemmell added a comment - Closing out

            People

            • Assignee:
              Robbie Gemmell
              Reporter:
              Rob Godfrey
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 6h
                6h
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 6h
                6h

                  Development