Details

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

      Description

      As was done for the Java client at https://issues.apache.org/jira/browse/QPID-3610. This is widely held to be a better default because it improves performance in a broader set of cases than the converse.

        Activity

        Hide
        Pavel Moravec added a comment -

        When implementing this improvement, please make --tcp-nodelay option similar to --auth (i.e. one can specify if the option is "on" or "off"). Current behavior is quite misleading:

        $ qpidd --trace --tcp-nodelay=no
        2013-01-09 12:24:37 [System] debug Exception constructed: Error in command line options: extra parameter in 'tcp-nodelay'

        $ qpidd --trace --tcp-nodelay no
        ..
        (tcp_nodelay is switched ON)

        $ qpidd --trace --tcp-nodelay false
        ..
        (tcp_nodelay is switched ON)

        $ qpidd --trace --tcp-nodelay=false
        2013-01-09 12:29:37 [System] debug Exception constructed: Error in command line options: extra parameter in 'tcp-nodelay'

        $ qpidd --trace
        ..
        (tcp_nodelay is switched off)

        Isn't it enough to apply this "patch" for this?

        — ./src/qpid/broker/Broker.cpp_orig 2013-01-09 12:31:22.599030546 +0100
        +++ ./src/qpid/broker/Broker.cpp_new 2013-01-09 12:31:44.028904499 +0100
        @@ -158,7 +158,7 @@ Broker::Options::Options(const std::stri
        ("auth", optValue(auth, "yes|no"), "Enable authentication, if disabled all incoming connections will be trusted")
        ("realm", optValue(realm, "REALM"), "Use the given realm when performing authentication")
        ("default-queue-limit", optValue(queueLimit, "BYTES"), "Default maximum size for queues (in bytes)")

        • ("tcp-nodelay", optValue(tcpNoDelay), "Set TCP_NODELAY on TCP connections")
          + ("tcp-nodelay", optValue(tcpNoDelay, "yes|no"), "Set TCP_NODELAY on TCP connections")
          ("require-encryption", optValue(requireEncrypted), "Only accept connections that are encrypted")
          ("known-hosts-url", optValue(knownHosts, "URL or 'none'"), "URL to send as 'known-hosts' to clients ('none' implies empty list)")
          ("sasl-config", optValue(saslConfigPath, "DIR"), "gets sasl config info from nonstandard location")
        Show
        Pavel Moravec added a comment - When implementing this improvement, please make --tcp-nodelay option similar to --auth (i.e. one can specify if the option is "on" or "off"). Current behavior is quite misleading: $ qpidd --trace --tcp-nodelay=no 2013-01-09 12:24:37 [System] debug Exception constructed: Error in command line options: extra parameter in 'tcp-nodelay' $ qpidd --trace --tcp-nodelay no .. (tcp_nodelay is switched ON ) $ qpidd --trace --tcp-nodelay false .. (tcp_nodelay is switched ON ) $ qpidd --trace --tcp-nodelay=false 2013-01-09 12:29:37 [System] debug Exception constructed: Error in command line options: extra parameter in 'tcp-nodelay' $ qpidd --trace .. (tcp_nodelay is switched off) Isn't it enough to apply this "patch" for this? — ./src/qpid/broker/Broker.cpp_orig 2013-01-09 12:31:22.599030546 +0100 +++ ./src/qpid/broker/Broker.cpp_new 2013-01-09 12:31:44.028904499 +0100 @@ -158,7 +158,7 @@ Broker::Options::Options(const std::stri ("auth", optValue(auth, "yes|no"), "Enable authentication, if disabled all incoming connections will be trusted") ("realm", optValue(realm, "REALM"), "Use the given realm when performing authentication") ("default-queue-limit", optValue(queueLimit, "BYTES"), "Default maximum size for queues (in bytes)") ("tcp-nodelay", optValue(tcpNoDelay), "Set TCP_NODELAY on TCP connections") + ("tcp-nodelay", optValue(tcpNoDelay, "yes|no"), "Set TCP_NODELAY on TCP connections") ("require-encryption", optValue(requireEncrypted), "Only accept connections that are encrypted") ("known-hosts-url", optValue(knownHosts, "URL or 'none'"), "URL to send as 'known-hosts' to clients ('none' implies empty list)") ("sasl-config", optValue(saslConfigPath, "DIR"), "gets sasl config info from nonstandard location")
        Hide
        Weston M. Price added a comment -

        Rajith probably has some comments on this. I know we had a long debate about this a few months ago.

        Show
        Weston M. Price added a comment - Rajith probably has some comments on this. I know we had a long debate about this a few months ago.
        Hide
        Andrew Stitcher added a comment -

        Unfortunately Pavel's patch is not good enough as it doesn't preserve backwards compatibility with old command lines. So any existing scripts will fail to work.

        The best solution I can find is to change the switch type of option (--tcp-nodelay & --quit etc.) so that they will take an optional parameter but they don't need to specify a parameter. This is only possible with newer versions of boost than 1.35. Which means that we can't do this for any distros that use an earlier version (RHEL5 is an example).

        However even without this change you could still "undefault" tcp nodelay by using the config file or environment variables.

        Show
        Andrew Stitcher added a comment - Unfortunately Pavel's patch is not good enough as it doesn't preserve backwards compatibility with old command lines. So any existing scripts will fail to work. The best solution I can find is to change the switch type of option (--tcp-nodelay & --quit etc.) so that they will take an optional parameter but they don't need to specify a parameter. This is only possible with newer versions of boost than 1.35. Which means that we can't do this for any distros that use an earlier version (RHEL5 is an example). However even without this change you could still "undefault" tcp nodelay by using the config file or environment variables.
        Hide
        Andrew Stitcher added a comment -

        This is fixed on trunk in r1469088

        Show
        Andrew Stitcher added a comment - This is fixed on trunk in r1469088
        Hide
        Andrew Stitcher added a comment -

        There is a fix on trunk to the original change in r1469466

        Show
        Andrew Stitcher added a comment - There is a fix on trunk to the original change in r1469466
        Hide
        Justin Ross added a comment -
        Show
        Justin Ross added a comment - Released in Qpid 0.24, http://qpid.apache.org/releases/qpid-0.24/index.html

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Justin Ross
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development