Qpid
  1. Qpid
  2. QPID-2796

[Java] implement support for heartbeats following IO changes to the 0-8/0-9/0-9-1/0-10 broker and 0-8/0-9/0-9-1 client

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.15
    • Fix Version/s: 0.20, 0.21
    • Component/s: Java Broker, Java Client
    • Labels:
      None

      Description

      The Java broker and client are not able to generate AMQP heartbeats following the updates to their IO layer. The 0-8/0-9/0-9-1 codepaths previously relied on the Mina IO layer to support this, and as such can no longer do so. The 0-10 path of the Java broker leverages much of the 0-10 implementation in common originally developed for the 0-10 Java client, which simply reflects heartbeats it receives from a broker and as such no heartbeats will ever be sent by broker or the client despite its support.

        Activity

        Hide
        Rob Godfrey added a comment -

        Merged to 0.20 as r1414927.

        Show
        Rob Godfrey added a comment - Merged to 0.20 as r1414927.
        Hide
        Justin Ross added a comment -

        Reviewed by Robbie. Approved for 0.20.

        Show
        Justin Ross added a comment - Reviewed by Robbie. Approved for 0.20.
        Hide
        Robbie Gemmell added a comment -

        Looks good to me.

        Show
        Robbie Gemmell added a comment - Looks good to me.
        Hide
        Rob Godfrey added a comment -

        I've made a further check-in to address Robbie's review comments:

        http://svn.apache.org/viewvc?rev=1413549&view=rev

        Show
        Rob Godfrey added a comment - I've made a further check-in to address Robbie's review comments: http://svn.apache.org/viewvc?rev=1413549&view=rev
        Hide
        Rob Godfrey added a comment -
        Show
        Rob Godfrey added a comment - Added a system test: http://svn.apache.org/viewvc?rev=1413539&view=rev
        Hide
        Robbie Gemmell added a comment -

        Changes look good so far. A couple of nitpicks and a posible problem:

        In the 0-10 Connection class there was a private implementation of TransportActivity added, but it isnt used. The updated connect() call passes null, which looks unsafe at first when following the subsequent method calls in IoNetowrkTransport but ultimately ends up being ok (because we dont set the reader/writer idle times at all on the 0-10 client side and rely on reflecting the brokers heartbeats if any) but possibly means we are doing socket timesouts with no effect. Given the IoReceiver ignores the ticker if it is null, could we just not create one if we arent going to really use it (as happens in the existing but now-unused IoNetworkConnection constructor)?

        In IdleTimeoutTicker it might be nicer if instead of checking !=0 it used >0 as happens elsewhere.

        Since IdleTimeoutTicker.tick() is only called when we get a SocketTimeoutException in IoReceiver, doesnt that mean its possible we can be in a writer-idle situation and yet never signal so (if the reader keeps getting data within its timeout, which is twice as long as the senders)? The old 0-8/9/9-1 clients will happily kill the connection in this situation if the broker doenst send them anything.

        Show
        Robbie Gemmell added a comment - Changes look good so far. A couple of nitpicks and a posible problem: In the 0-10 Connection class there was a private implementation of TransportActivity added, but it isnt used. The updated connect() call passes null, which looks unsafe at first when following the subsequent method calls in IoNetowrkTransport but ultimately ends up being ok (because we dont set the reader/writer idle times at all on the 0-10 client side and rely on reflecting the brokers heartbeats if any) but possibly means we are doing socket timesouts with no effect. Given the IoReceiver ignores the ticker if it is null, could we just not create one if we arent going to really use it (as happens in the existing but now-unused IoNetworkConnection constructor)? In IdleTimeoutTicker it might be nicer if instead of checking !=0 it used >0 as happens elsewhere. Since IdleTimeoutTicker.tick() is only called when we get a SocketTimeoutException in IoReceiver, doesnt that mean its possible we can be in a writer-idle situation and yet never signal so (if the reader keeps getting data within its timeout, which is twice as long as the senders)? The old 0-8/9/9-1 clients will happily kill the connection in this situation if the broker doenst send them anything.
        Hide
        Rob Godfrey added a comment -

        Added support for sending heartbeats to the Java Broker (all 0-x protocol versions) and Java Client (0-8/9/9-1 protocols).

        http://svn.apache.org/viewvc?rev=1413376&view=rev

        Will be adding a system test before resolving the JIRA

        Show
        Rob Godfrey added a comment - Added support for sending heartbeats to the Java Broker (all 0-x protocol versions) and Java Client (0-8/9/9-1 protocols). http://svn.apache.org/viewvc?rev=1413376&view=rev Will be adding a system test before resolving the JIRA
        Hide
        Robbie Gemmell added a comment -

        Issue updated to reflect scope widening from only the0-10 path being unable to use heartbeats to all protocols paths currently being unable to support heartbeats

        Show
        Robbie Gemmell added a comment - Issue updated to reflect scope widening from only the0-10 path being unable to use heartbeats to all protocols paths currently being unable to support heartbeats
        Hide
        Robbie Gemmell added a comment -

        Updating 'Fix For' to Unknown on issues not targeted for 0.8

        Show
        Robbie Gemmell added a comment - Updating 'Fix For' to Unknown on issues not targeted for 0.8

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Robbie Gemmell
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development