Solr
  1. Solr
  2. SOLR-5649

Small ConnectionManager improvements

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.6.1
    • Fix Version/s: 4.7, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      I was just looking through the ConnectionManager and want to jot these down before I forget them. I'm happy to make a patch if someone thinks it's valuable as well.

      • "clientConnected" doesn't seem to be read, can be eliminated
      • "state" is a private volatile variable, but only used in one function – seems unlikely private volatile is what is wanted
      • A comment explaining why disconnected() is not called in the case of Expired would be helpful (Expired means we have already waited the timeout period so we want to reject updates right away)

        Activity

        Hide
        Mark Miller added a comment -

        "clientConnected" doesn't seem to be read, can be eliminated

        Yup, a relica from original code.

        "state" is a private volatile variable, but only used in one function – seems unlikely private volatile is what is wanted

        Yeah, there used to be a getState method that we inherited but it is gone now.

        A comment explaining why disconnected() is not called in the case of Expired would be helpful (Expired means we have already waited the timeout period so we want to reject updates right away)

        The current comment already says:

        // we don't call disconnected because there
        // is no need to start the timer - if we are expired
        // likelyExpired can just be set to true

        Show
        Mark Miller added a comment - "clientConnected" doesn't seem to be read, can be eliminated Yup, a relica from original code. "state" is a private volatile variable, but only used in one function – seems unlikely private volatile is what is wanted Yeah, there used to be a getState method that we inherited but it is gone now. A comment explaining why disconnected() is not called in the case of Expired would be helpful (Expired means we have already waited the timeout period so we want to reject updates right away) The current comment already says: // we don't call disconnected because there // is no need to start the timer - if we are expired // likelyExpired can just be set to true
        Hide
        Mark Miller added a comment - - edited

        A couple additional items:

        • We don't want to call connected() after updating the SolrZooKeeper instance in process - the new event thread will call on the connect event. We just want to start the reconnected thread and then that thread will process the thread death event and die.
        • Greg mentioned to me he also noticed the connected status variable was now in a non synchronized block - it should be synchronized or changed to a volatile.
        Show
        Mark Miller added a comment - - edited A couple additional items: We don't want to call connected() after updating the SolrZooKeeper instance in process - the new event thread will call on the connect event. We just want to start the reconnected thread and then that thread will process the thread death event and die. Greg mentioned to me he also noticed the connected status variable was now in a non synchronized block - it should be synchronized or changed to a volatile.
        Hide
        Mark Miller added a comment -

        A patch addressing the above issues.

        Show
        Mark Miller added a comment - A patch addressing the above issues.
        Hide
        ASF subversion and git services added a comment -

        Commit 1567399 from Mark Miller in branch 'dev/trunk'
        [ https://svn.apache.org/r1567399 ]

        SOLR-5649: Clean up some minor ConnectionManager issues.

        Show
        ASF subversion and git services added a comment - Commit 1567399 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1567399 ] SOLR-5649 : Clean up some minor ConnectionManager issues.
        Hide
        ASF subversion and git services added a comment -

        Commit 1567400 from Mark Miller in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1567400 ]

        SOLR-5649: Clean up some minor ConnectionManager issues.

        Show
        ASF subversion and git services added a comment - Commit 1567400 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1567400 ] SOLR-5649 : Clean up some minor ConnectionManager issues.
        Hide
        Mark Miller added a comment -

        Thanks Greg!

        Show
        Mark Miller added a comment - Thanks Greg!

          People

          • Assignee:
            Mark Miller
            Reporter:
            Gregory Chanan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development