Qpid
  1. Qpid
  2. QPID-2834

Implement subscriptions (SUB) operational logging on 0-10

    Details

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

      Description

      This is a subset of Qpid-2801 dealing only with subscriptions.

      1. qpid-2834.patch
        4 kB
        Sorin Suciu
      2. qpid-2834_1.patch
        1 kB
        Sorin Suciu

        Activity

        Hide
        Robbie Gemmell added a comment -

        There is a large amount of reformatting contained in the patch. Whilst I appreciate this was likely automated and seperating it would be desirable, the only bit I actually have an issue with is this:

        • /*properties.getHeaders().processOverElements(
        • new FieldTable.FieldTableElementProcessor()
        • {
          -
        • public boolean processElement(String propertyName, AMQTypedValue value)
        • {
        • Object val = value.getValue();
        • if(val instanceof AMQShortString)
        • { - val = val.toString(); - }
        • appHeaders.put(propertyName, val);
        • return true;
        • }
          -
        • public Object getResult()
        • { - return appHeaders; - }
        • });
          -
          -
        • messageProps.setApplicationHeaders(appHeaders);
          -*/
          + /*
          + * properties.getHeaders().processOverElements( new
          + * FieldTable.FieldTableElementProcessor() { public boolean
          + * processElement(String propertyName, AMQTypedValue value)
          Unknown macro:
          Unknown macro: { Object+ * val = value.getValue(); if(val instanceof AMQShortString) { val = + * val.toString(); } appHeaders.put(propertyName, val); return true;+ * }

          public Object getResult()

          { return appHeaders; }

          });
          + * messageProps.setApplicationHeaders(appHeaders);
          + */

        If the commented out code isnt of any use it should just be deleted.

        When logging the subscription state change, the patch uses _logActor,
        + _logActor.message(_logSubject, SubscriptionMessages.STATE(_state.get().toString()));

        The matching line in SubscriptionImpl uses CurrentActor.get() instead (probably because it may convey the queue whos async delivery thread caused the state state change, instead of the subscriptions own flusher). This applies to all of the logging in the Subscription's and they should be updated.

        Additionally, when checking if the subscription CREATE message is enabled as such:
        + if (_logActor.getRootMessageLogger().isMessageEnabled(_logActor, this, this.getClass().getCanonicalName()))

        it is not the class name that should be supplied for the log hierarchy, but the heirarchy associated with each individual log message (in the generated files). The example in SubscriptionImpl can be copied directly.

        Show
        Robbie Gemmell added a comment - There is a large amount of reformatting contained in the patch. Whilst I appreciate this was likely automated and seperating it would be desirable, the only bit I actually have an issue with is this: /*properties.getHeaders().processOverElements( new FieldTable.FieldTableElementProcessor() { - public boolean processElement(String propertyName, AMQTypedValue value) { Object val = value.getValue(); if(val instanceof AMQShortString) { - val = val.toString(); - } appHeaders.put(propertyName, val); return true; } - public Object getResult() { - return appHeaders; - } }); - - messageProps.setApplicationHeaders(appHeaders); -*/ + /* + * properties.getHeaders().processOverElements( new + * FieldTable.FieldTableElementProcessor() { public boolean + * processElement(String propertyName, AMQTypedValue value) Unknown macro: Unknown macro: { Object+ * val = value.getValue(); if(val instanceof AMQShortString) { val = + * val.toString(); } appHeaders.put(propertyName, val); return true;+ * } public Object getResult() { return appHeaders; } }); + * messageProps.setApplicationHeaders(appHeaders); + */ If the commented out code isnt of any use it should just be deleted. When logging the subscription state change, the patch uses _logActor, + _logActor.message(_logSubject, SubscriptionMessages.STATE(_state.get().toString())); The matching line in SubscriptionImpl uses CurrentActor.get() instead (probably because it may convey the queue whos async delivery thread caused the state state change, instead of the subscriptions own flusher). This applies to all of the logging in the Subscription's and they should be updated. Additionally, when checking if the subscription CREATE message is enabled as such: + if (_logActor.getRootMessageLogger().isMessageEnabled(_logActor, this, this.getClass().getCanonicalName())) it is not the class name that should be supplied for the log hierarchy, but the heirarchy associated with each individual log message (in the generated files). The example in SubscriptionImpl can be copied directly.
        Hide
        Sorin Suciu added a comment -

        Patch modified to address Robbie's comments.

        Show
        Sorin Suciu added a comment - Patch modified to address Robbie's comments.
        Hide
        Robbie Gemmell added a comment -

        You cant log messages in the constructor as the subscription information must know about the queue, which is set in the setQueue method (which is the point the creation is logged).

        The CREATE and CLOSE messages continue to use _logActor instead of CurrentActor.get(), they should be updated.

        The START message in the addCredit() method should only be emitted if the flow was previously stopped, there are situations where it will currently be emitted upon delivery of every message (regardless whether the subscription was stopped). There is an atomic boolean used in both addCredit() and the stop() methods already that can be used to ensure this condition. The STOP message should be emitted from the stop() method, it is currently not used at all except in the constructor.

        I would rename START and STOP to STARTED and STOPPED respectively and make the text indicate that it is discussing Flow (ie 'Flow Started' instead of 'Start', for consistency with the existing Channel messages).

        Looking at it there appears to be additional places in the class that state changes from active to suspended, and these seem to call the currently-unused state listenener. It may be best to move all possible logging into the state listener to ensure no changes are missed.

        START/STOP should probably use CurrentActor.get()

        Where are FLOW message IDs 1002 and 1004 anyway?

        The ENFORCED and REMOVED messages dont really convey that it is (Producer) Flow Control that is being discussed. In the Channel implementation it is possible to say whcih queue caused the Channel to have flow control enforced, for 0-10 it should be possible to say this for subscriptions instead (since flow control is per subscription and not session/channel)....however, as they arent being used they should just be deleted anyway. Same for RESTORED and MODE.

        That would leave only the start/stop messages, and given that these messages are about Subscriptions (albeit 0-10 only) I think theyd be best put alogn with the Subscription log messages.

        Show
        Robbie Gemmell added a comment - You cant log messages in the constructor as the subscription information must know about the queue, which is set in the setQueue method (which is the point the creation is logged). The CREATE and CLOSE messages continue to use _logActor instead of CurrentActor.get(), they should be updated. The START message in the addCredit() method should only be emitted if the flow was previously stopped, there are situations where it will currently be emitted upon delivery of every message (regardless whether the subscription was stopped). There is an atomic boolean used in both addCredit() and the stop() methods already that can be used to ensure this condition. The STOP message should be emitted from the stop() method, it is currently not used at all except in the constructor. I would rename START and STOP to STARTED and STOPPED respectively and make the text indicate that it is discussing Flow (ie 'Flow Started' instead of 'Start', for consistency with the existing Channel messages). Looking at it there appears to be additional places in the class that state changes from active to suspended, and these seem to call the currently-unused state listenener. It may be best to move all possible logging into the state listener to ensure no changes are missed. START/STOP should probably use CurrentActor.get() Where are FLOW message IDs 1002 and 1004 anyway? The ENFORCED and REMOVED messages dont really convey that it is (Producer) Flow Control that is being discussed. In the Channel implementation it is possible to say whcih queue caused the Channel to have flow control enforced, for 0-10 it should be possible to say this for subscriptions instead (since flow control is per subscription and not session/channel)....however, as they arent being used they should just be deleted anyway. Same for RESTORED and MODE. That would leave only the start/stop messages, and given that these messages are about Subscriptions (albeit 0-10 only) I think theyd be best put alogn with the Subscription log messages.
        Hide
        Sorin Suciu added a comment -

        Closing a SUB needs an actor on the CurrentActor stack.

        Show
        Sorin Suciu added a comment - Closing a SUB needs an actor on the CurrentActor stack.
        Hide
        Andrew Kennedy added a comment -

        Committed latest patch from SorinS <ssuciu@gmail.com>

        Show
        Andrew Kennedy added a comment - Committed latest patch from SorinS <ssuciu@gmail.com>

          People

          • Assignee:
            Andrew Kennedy
            Reporter:
            Sorin Suciu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development