Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.19
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      == Background ==

      I've been looking at what would be required to get AMQP 1.0 support in
      the qpidd broker (using proton-c). In that context I felt there was a
      need to refactor the broker code, particularly that part that would be
      shared between different protocol versions. Part of the motivation was
      clearer separation of 0-10 specific logic, so that 1.0 logic could be
      introduced as an alternative. However part of it was also simply the
      recognition of some long-standing problems that we have never stopped
      to address.

      So, here is a patch representing my ideas on what is needed. This is
      a little stale again (patch was generated against r13613342) and
      needs (yet) another rebase. However it is getting to the point where I'll be asking to commit it soon, so if anyone has feedback, now is the time to give it!

      == Key Changes ==

      qpid::broker::Message

      This is now supposed to be a protocol neutral representation of a
      message. It no longer exposes qpid::framing::FrameSet. It can be based
      on data received in different encodings (this patch only includes the
      existing 0-10 encoding).

      The immutable, sharable state is separated from the mutable
      queue-specific state. Messages themselves are no longer held through a
      shared pointer but are passed by reference or copied if needed. The
      immutable state (essentially the data as received) is still shared
      and referenced internally through an intrusive pointer. There is no
      longer a message level lock. A message instance is 'owned' by
      someother entity (usually the queue it is on) which controls
      concurrent access/modification if necessary.

      The persistence context is a separate part of the message
      also. Currently that can be shared between two message instances if
      desired.

      qpid::broker::Messages

      Switched from using qpid::broker::QueuedMessage (which relied on
      shared pointer to message itself and made sequence number the explicit

      • and only - way to refer to a specific message) to using modified
        Message class directly and a new qpid::broker::QueueCursor.

      The cursor is opaque outside the Messages implementation to which it
      relates. It provides a way to refer to a specific message (without
      directly using sequence number, though at present that is what is used
      'under the covers') and/or to track progress through a sequence of
      messages (for consumers or other iterating entities).

      I.e. its an iterator to a Message within its containing Messages
      instance that is not invalidated by changes to that container.

      A Messages instance owns the Message instances within it. Other
      classes access this through a reference or (raw) pointer, or if needed
      copy it (the immutable part can be - and is - safely shared).

      The codepath for browse/consume is a lot more unified now. You use a
      cursor and call Messages::next() in each case. This also lays the
      foundation for selectors.

      The simplified Messages interface led to a simplied
      MessageDistributor. There is still a little more to do to clarify
      these separate roles (or indeed perhaps unify them?) but more on that
      later.

      qpid::broker::amqp_0_10::MessageTransfer

      This represents the familiar 0-10 encoding of a message. This class is
      broadly similar to the old Message class, based on a FrameSet. However
      it represents the shared and essentially immutable state. The
      sendHeader() method now explicitly takes a copy of the original
      headers and adds to it or otherwise modifies it if needed (e.g. for
      redelivered flag, ttl, annotations etc).

      [Ideally I'd like to move more of the 0-10 specific classes out of
      qpid::broker and into qpid::broker::amqp_0_10, but that has no
      functional relevance so I've left existing classes alone for now.]

      qpid::broker::Consumer

      The deliver() method now takes a QueueCursor (representing a 'handle'
      to this message for use in subsequent operations such as accept,
      relese etc) and a constant reference to the Message itself
      (i.e. consumers can't alter the state of the message on the queue
      directly, but only through operations on the queue itself).

      qpid::broker::QueueRegistry

      The actual queue creation has been pulled out into a base class,
      QueueFactory. The actual class of the Queue returned can now be varied
      and there are two subclasses in the current patch. The first is a
      replacement for the ring policy logic, whereby messages are removed
      from the queue in order to keep the queue from growing above a
      configured limit. The second is for last value queues and simply pulls
      the special case behaviour out of the common code path.

      The handling of queue configuration has also been made cleaner and
      more uniform, based on the QueueSettings class.

      qpid::broker::QueuePolicy

      This class has been removed. There is a new QueueDepth utility used
      for configuring limits, tracking current depth and testing the latter
      against the former. This is used directly by Queue. The behaviour at
      the limit can be varied by subclassing queue.

      == Limitations etc ==

      clustering

      This breaks clustering. Indeed it will not compile unless clustering
      is disabled (--without-cpg in configure). Keeping the cluster code in
      sync was distracting me from the core goal, given its entanglement
      with the broker code.

      My assumption is that the new ha code will eventually replace the
      cluster anyway and the amount of change that would be required to get
      the cluster working with this refactor may not be worth it and may in
      fact undermine its stability anyway (which seem the only good argument
      for using it).

      I don't believe there is anything insurmountable to do to re-enable
      cluster if that was desired however, just a fair chunk more work that I would rather not do if I can avoid it.

      old & nasty features removed

      I have removed support for flow to disk, the legacy version of lvq
      with two modes (the updated version of lvq is of course still
      functional), the last-man-standing persistence in clustering and the
      old async queue replication. They are really quite horribly
      implemented and/or are no longer necessary in my view.

        Activity

        Hide
        Gordon Sim added a comment -
        Show
        Gordon Sim added a comment - See https://reviews.apache.org/r/5833/ for patch
        Hide
        Gordon Sim added a comment -

        Patch for external store plugin inline with this refactor; gets all tests working (removes all flow to disk tests).

        Show
        Gordon Sim added a comment - Patch for external store plugin inline with this refactor; gets all tests working (removes all flow to disk tests).
        Hide
        Gordon Sim added a comment -

        Slight tweak to previous patch, avoiding unintentional commenting out of subdirs for tests.

        Show
        Gordon Sim added a comment - Slight tweak to previous patch, avoiding unintentional commenting out of subdirs for tests.
        Hide
        Keith Wall added a comment -

        Hello Gordon

        The change to the Python test GeneralTests.test_qpid_3481_acquired_to_alt_exchange_2_consumers is failing against the Java Broker. See:

        https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-Python-Java-Test/313/testReport/

        I'm curious regarding the new behaviour of the CPP Broker. It seems to be inferior to the old in that the client application is now exposed to the possibility of duplicates (in the case highlight by the test). What is the rationale behind this change?

        If the new behaviour CPP is desired, could you change the test so that the test can continue to pass against the Java Broker too.

        regards, Keith.

        Show
        Keith Wall added a comment - Hello Gordon The change to the Python test GeneralTests.test_qpid_3481_acquired_to_alt_exchange_2_consumers is failing against the Java Broker. See: https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-Python-Java-Test/313/testReport/ I'm curious regarding the new behaviour of the CPP Broker. It seems to be inferior to the old in that the client application is now exposed to the possibility of duplicates (in the case highlight by the test). What is the rationale behind this change? If the new behaviour CPP is desired, could you change the test so that the test can continue to pass against the Java Broker too. regards, Keith.
        Hide
        Gordon Sim added a comment -

        I would consider the original behaviour of the c++ broker to be a bug, though I concede it is a grey area in the spec. The spec states: "When a queue is deleted any pending messages are sent to the alternate-exchange if defined, or discarded if it is not." It doesn't describe what 'pending messages' means in this case.

        I think the most logical interpretation is for that to mean all enqueued messages; if the queue is deleted they are all 'orphaned'. I do think that explicitly deleting a queue from which you have unacknowledged messages is a bizarre thing to do. In the case of exclusive-auto-delete, session termination would both delete the queue and release all acquired messages. Auto-delete of a shared queue in use by other sessions seems like a subset of the explicit case to me.

        So, in summary, I think the test (in both its original and revised form) relies on unspecified behaviour. I myself think the revised behaviour makes much more sense and would be happy to keep that while excluding it for the Java broker, or delete it entirely if you disagree.

        Thoughts?

        Show
        Gordon Sim added a comment - I would consider the original behaviour of the c++ broker to be a bug, though I concede it is a grey area in the spec. The spec states: "When a queue is deleted any pending messages are sent to the alternate-exchange if defined, or discarded if it is not." It doesn't describe what 'pending messages' means in this case. I think the most logical interpretation is for that to mean all enqueued messages; if the queue is deleted they are all 'orphaned'. I do think that explicitly deleting a queue from which you have unacknowledged messages is a bizarre thing to do. In the case of exclusive-auto-delete, session termination would both delete the queue and release all acquired messages. Auto-delete of a shared queue in use by other sessions seems like a subset of the explicit case to me. So, in summary, I think the test (in both its original and revised form) relies on unspecified behaviour. I myself think the revised behaviour makes much more sense and would be happy to keep that while excluding it for the Java broker, or delete it entirely if you disagree. Thoughts?
        Hide
        Keith Wall added a comment -

        On reflection, I think you are correct, the new behaviour seems to be the most reasonable interpretation of the spec. I'm happy the the test to be excluded from the Java Broker (I see you've done this already). Thanks for the explanation. Keith.

        Show
        Keith Wall added a comment - On reflection, I think you are correct, the new behaviour seems to be the most reasonable interpretation of the spec. I'm happy the the test to be excluded from the Java Broker (I see you've done this already). Thanks for the explanation. Keith.

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development