Uploaded image for project: 'Qpid'
  1. Qpid
  2. QPID-4124

[Java Broker] TransactionTimeout logging has duplication and some erroneously uses default Object.toString

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.14, 0.15, 0.16, 0.17, 0.18
    • Fix Version/s: 0.19
    • Component/s: Java Broker
    • Labels:
      None

      Description

      In AMQChannel, code such as

      _logger.warn("IDLE TRANSACTION ALERT " + _logSubject.toString() ...

      should be replaced by

      _logger.warn("IDLE TRANSACTION ALERT " + _logSubject.toLogString() ...

      because the first example uses Object.toString, which is not very useful for logging.

        Activity

        Hide
        philharveyonline Philip Harvey added a comment -

        attached patch

        Show
        philharveyonline Philip Harvey added a comment - attached patch
        Hide
        philharveyonline Philip Harvey added a comment -

        please review and commit if you're happy.

        Show
        philharveyonline Philip Harvey added a comment - please review and commit if you're happy.
        Hide
        philharveyonline Philip Harvey added a comment -

        After further discussion, we've decided that this logging should only be produced if operational logging is switched off (to avoid duplication of log messages).

        Show
        philharveyonline Philip Harvey added a comment - After further discussion, we've decided that this logging should only be produced if operational logging is switched off (to avoid duplication of log messages).
        Hide
        alex.rufous Alex Rudyy added a comment -

        Attached patch allowing to have either operational or standard logs to avoid duplication in transaction timeout functionality on 0-9.x and 0.10 code paths.

        Show
        alex.rufous Alex Rudyy added a comment - Attached patch allowing to have either operational or standard logs to avoid duplication in transaction timeout functionality on 0-9.x and 0.10 code paths.
        Hide
        gemmellr Robbie Gemmell added a comment -

        Keith and I reviewed the patch, and have made some modifications before committing it:

        • Use an instance of the helper per-channel instead of creating 2 new ones every time the check is performed.
        • Add some tests for the case when the timeouts are 0, and the operational logging is disabled.
        • (issue with the existing code) Move the closures right after the respective idle/open checks such that you only get the relevant log messages before the close operation is performed, reducing duplication.
        Show
        gemmellr Robbie Gemmell added a comment - Keith and I reviewed the patch, and have made some modifications before committing it: Use an instance of the helper per-channel instead of creating 2 new ones every time the check is performed. Add some tests for the case when the timeouts are 0, and the operational logging is disabled. (issue with the existing code) Move the closures right after the respective idle/open checks such that you only get the relevant log messages before the close operation is performed, reducing duplication.

          People

          • Assignee:
            philharveyonline Philip Harvey
            Reporter:
            philharveyonline Philip Harvey
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development