Cassandra
  1. Cassandra
  2. CASSANDRA-488

TcpConnectionManager only ever has one connection

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 0.5
    • Component/s: None
    • Labels:
      None

      Description

      from irc:

      jbellis: am i missing something? it looks like TcpConnectionManager.allConnections_ will only ever contain a single TcpConnection, since before adding any connection it checks contains() first, which relies on TcpConn.equals, which reports that any TcpConns w/ same local & remote endpoints are equal

      junrao: yes, TcpConnectionManager.allConnections_ can have no more than 1 element.

      jbellis: that might explain why it's so easy to swamp things and start getting timeouts – if all your connections are full of command data, "success" replies will have to wait for those to drain out first before being sent

      IMO the Right Design is one connection for commands, one connection for acks/replies, rather than a single pool of connections for everything. that way a connection from A -> B "full" of outgoing commands can't block replies from A -> B acking commands that came the other way.

      1. 488-v3.patch
        18 kB
        Jonathan Ellis
      2. 488-v5.patch
        17 kB
        Gary Dusbabek
      3. cassandra-488-v1.patch
        15 kB
        Gary Dusbabek
      4. cassandra-488-v2.patch
        17 kB
        Gary Dusbabek
      5. cassandra-488-v4.patch
        17 kB
        Gary Dusbabek

        Activity

        Hide
        Jun Rao added a comment -

        Also, TcpConnectionManager today is initialized with a maxSize of 1. This guarantees that some ordering in message delivery. Not sure if anything depends on this assumption though.

        Show
        Jun Rao added a comment - Also, TcpConnectionManager today is initialized with a maxSize of 1. This guarantees that some ordering in message delivery. Not sure if anything depends on this assumption though.
        Hide
        Jonathan Ellis added a comment -

        good point, that is another reason I don't think the "pool of connections" is a good choice – it makes it a lot harder to reason about ordering.

        Show
        Jonathan Ellis added a comment - good point, that is another reason I don't think the "pool of connections" is a good choice – it makes it a lot harder to reason about ordering.
        Hide
        Hudson added a comment -

        Integrated in Cassandra #235 (See http://hudson.zones.apache.org/hudson/job/Cassandra/235/)
        clean up unused code; add comments. patch by jbellis for
        reformat whitespace. patch by jbellis for

        Show
        Hudson added a comment - Integrated in Cassandra #235 (See http://hudson.zones.apache.org/hudson/job/Cassandra/235/ ) clean up unused code; add comments. patch by jbellis for reformat whitespace. patch by jbellis for
        Hide
        Gary Dusbabek added a comment -

        There is a lot of unused cruft in TcpConnectionManager that can be removed.

        Show
        Gary Dusbabek added a comment - There is a lot of unused cruft in TcpConnectionManager that can be removed.
        Hide
        Jonathan Ellis added a comment -

        that's the idea

        Show
        Jonathan Ellis added a comment - that's the idea
        Hide
        Gary Dusbabek added a comment -

        I felt like removing MessagingConfig altogether, but figured it was there for a purpose.

        Show
        Gary Dusbabek added a comment - I felt like removing MessagingConfig altogether, but figured it was there for a purpose.
        Hide
        Gary Dusbabek added a comment -

        Ready for review.

        Show
        Gary Dusbabek added a comment - Ready for review.
        Hide
        Jonathan Ellis added a comment -

        i vote for r/m MC too

        Show
        Jonathan Ellis added a comment - i vote for r/m MC too
        Hide
        Gary Dusbabek added a comment -

        MessagingConfig is no more.

        Show
        Gary Dusbabek added a comment - MessagingConfig is no more.
        Hide
        Jonathan Ellis added a comment -

        reformatted a couple things here.

        let's use synchronized instead of lock in TCM, and add a method to null a cached conn back out for use in TC.close / errorClose.

        (may not be necessary in close, i think that's only used in "streaming" conns which are never one of the cached ones. but haven't looked too closely.)

        Show
        Jonathan Ellis added a comment - reformatted a couple things here. let's use synchronized instead of lock in TCM, and add a method to null a cached conn back out for use in TC.close / errorClose. (may not be necessary in close, i think that's only used in "streaming" conns which are never one of the cached ones. but haven't looked too closely.)
        Hide
        Jonathan Ellis added a comment -

        (correct attachment here)

        Show
        Jonathan Ellis added a comment - (correct attachment here)
        Hide
        Gary Dusbabek added a comment -

        converted lock to synchronized.

        Show
        Gary Dusbabek added a comment - converted lock to synchronized.
        Hide
        Gary Dusbabek added a comment -

        nulls connections on errorClose. I didn't bother on close(), since it looks like close() really indicates that the connection is not currently being used rather than dead or shutdown.

        Show
        Gary Dusbabek added a comment - nulls connections on errorClose. I didn't bother on close(), since it looks like close() really indicates that the connection is not currently being used rather than dead or shutdown.
        Hide
        Jonathan Ellis added a comment -

        committed

        Show
        Jonathan Ellis added a comment - committed
        Hide
        Hudson added a comment -

        Integrated in Cassandra #254 (See http://hudson.zones.apache.org/hudson/job/Cassandra/254/)
        clean out unused code from TcpConnectionManager; split connections to a node into "command" and "ack", which will allow us to use backpressure on the command socket. patch by gdusbabek; reviewed by jbellis for

        Show
        Hudson added a comment - Integrated in Cassandra #254 (See http://hudson.zones.apache.org/hudson/job/Cassandra/254/ ) clean out unused code from TcpConnectionManager; split connections to a node into "command" and "ack", which will allow us to use backpressure on the command socket. patch by gdusbabek; reviewed by jbellis for

          People

          • Assignee:
            Gary Dusbabek
            Reporter:
            Jonathan Ellis
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development