Qpid
  1. Qpid
  2. QPID-2839

Implement Channel (CHN) operational logging on 0-10 protocol

    Details

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

      Description

      This is a part of Qpid-2801dealing with CHN operational logging on 0-10

        Activity

        Hide
        Robbie Gemmell added a comment -

        The patch adds commented out log statements, they should be removed:

        @@ -658,6 +665,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr
        public void restoreCredit(QueueEntry queueEntry)

        { _creditManager.restoreCredit(1, queueEntry.getSize()); + // _logActor.message(FlowMessages.RESTORED()); }

        public void onDequeue(QueueEntry queueEntry)
        @@ -719,6 +727,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr
        _stopped.set(true);
        FlowCreditManager_0_10 creditManager = getCreditManager();
        creditManager.clearCredit();
        + // _logActor.message(FlowMessages.REMOVED());
        }

        @@ -658,6 +665,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr
        public void restoreCredit(QueueEntry queueEntry){ _creditManager.restoreCredit(1, queueEntry.getSize()); + // _logActor.message(FlowMessages.RESTORED()); }

        }

        public void onDequeue(QueueEntry queueEntry)
        @@ -719,6 +727,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr
        _stopped.set(true);
        FlowCreditManager_0_10 creditManager = getCreditManager();
        creditManager.clearCredit();
        + // _logActor.message(FlowMessages.REMOVED());
        }

        In the Subscription_0_10 toLogString() method, the updated queueInfo string generation seems like it could be moved elsewhere to prevent doign expensive string operations upon every output of every log message. As you are not using the QueueLogSubject to acquire the queue information it isnt clear that it should be trimming the result. You are also not using the static log format string for the queue (as added/moved in the QPID-2832 patch specifically to make their relationships more clear and to statically import from).

        It isnt clear why there are ENFORCED messages in the addCredit() method. The old FLOW_ENFORCED message for the channel relates to the application of Producer Side Flow Control, which isnt supported in the 0-10 implementation yet and also does not relate to how much credit is available. The addCredit() method then goes on to output the new FlowMessage.START if the subscription becomes active again, but the active/suspended state message to signal this type of change already exist.

        The section containing the above changes should also really be in the patch for subscriptions, ie QPID-2394.

        Show
        Robbie Gemmell added a comment - The patch adds commented out log statements, they should be removed: @@ -658,6 +665,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr public void restoreCredit(QueueEntry queueEntry) { _creditManager.restoreCredit(1, queueEntry.getSize()); + // _logActor.message(FlowMessages.RESTORED()); } public void onDequeue(QueueEntry queueEntry) @@ -719,6 +727,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr _stopped.set(true); FlowCreditManager_0_10 creditManager = getCreditManager(); creditManager.clearCredit(); + // _logActor.message(FlowMessages.REMOVED()); } @@ -658,6 +665,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr public void restoreCredit(QueueEntry queueEntry){ _creditManager.restoreCredit(1, queueEntry.getSize()); + // _logActor.message(FlowMessages.RESTORED()); } } public void onDequeue(QueueEntry queueEntry) @@ -719,6 +727,7 @@ public class Subscription_0_10 implements Subscription, FlowCreditManager.FlowCr _stopped.set(true); FlowCreditManager_0_10 creditManager = getCreditManager(); creditManager.clearCredit(); + // _logActor.message(FlowMessages.REMOVED()); } In the Subscription_0_10 toLogString() method, the updated queueInfo string generation seems like it could be moved elsewhere to prevent doign expensive string operations upon every output of every log message. As you are not using the QueueLogSubject to acquire the queue information it isnt clear that it should be trimming the result. You are also not using the static log format string for the queue (as added/moved in the QPID-2832 patch specifically to make their relationships more clear and to statically import from). It isnt clear why there are ENFORCED messages in the addCredit() method. The old FLOW_ENFORCED message for the channel relates to the application of Producer Side Flow Control, which isnt supported in the 0-10 implementation yet and also does not relate to how much credit is available. The addCredit() method then goes on to output the new FlowMessage.START if the subscription becomes active again, but the active/suspended state message to signal this type of change already exist. The section containing the above changes should also really be in the patch for subscriptions, ie QPID-2394 .
        Hide
        Sorin Suciu added a comment -

        Current patch to address Robbie's comments.

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

        CLOSE should use CurrentActor.get() instead of _logActor

        Show
        Robbie Gemmell added a comment - CLOSE should use CurrentActor.get() instead of _logActor
        Hide
        Sorin Suciu added a comment -

        Modified so that ChannelMessages.CLOSE() use the CurrentActor.get()

        Show
        Sorin Suciu added a comment - Modified so that ChannelMessages.CLOSE() use the CurrentActor.get()
        Hide
        Andrew Kennedy added a comment -

        Reviewed patch with SorinS <ssuciu@gmail.com> and accepted suggested changes

        Show
        Andrew Kennedy added a comment - Reviewed patch with SorinS <ssuciu@gmail.com> and accepted suggested changes
        Hide
        Andrew Kennedy added a comment -

        Patch from SorinS <ssuciu@gmail.com> with review changes

        Show
        Andrew Kennedy added a comment - Patch from SorinS <ssuciu@gmail.com> with review changes
        Hide
        Andrew Kennedy added a comment -

        Committed changes

        Show
        Andrew Kennedy added a comment - Committed changes

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development