Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-488

TcpConnectionManager only ever has one connection

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: 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
        junrao 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
        junrao 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
        jbellis 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
        jbellis 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 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 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
        gdusbabek Gary Dusbabek added a comment -

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

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

        that's the idea

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

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

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

        Ready for review.

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

        i vote for r/m MC too

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

        MessagingConfig is no more.

        Show
        gdusbabek Gary Dusbabek added a comment - MessagingConfig is no more.
        Hide
        jbellis 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
        jbellis 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
        jbellis Jonathan Ellis added a comment -

        (correct attachment here)

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

        converted lock to synchronized.

        Show
        gdusbabek Gary Dusbabek added a comment - converted lock to synchronized.
        Hide
        gdusbabek 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
        gdusbabek 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
        jbellis Jonathan Ellis added a comment -

        committed

        Show
        jbellis Jonathan Ellis added a comment - committed
        Hide
        hudson 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 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:
            gdusbabek Gary Dusbabek
            Reporter:
            jbellis Jonathan Ellis
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development