Solr
  1. Solr
  2. SOLR-6793

ReplicationHandler does not destroy all of it's created SnapPullers

    Details

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

      Issue Links

        Activity

        Hide
        Mark Miller added a comment -

        I don't really think this is much of a current problem - but as we build and add, we would like to know that destroy is called for each SnapPuller for cleanup.

        Show
        Mark Miller added a comment - I don't really think this is much of a current problem - but as we build and add, we would like to know that destroy is called for each SnapPuller for cleanup.
        Hide
        Ramkumar Aiyengar added a comment -

        +1 for the current patch, it does fix stuff, but in doFetch, shouldn't the finally block have the same cleanup?

        FWIW, this entire (temp)SnapPuller stuff is pretty messy, took me quite a bit of time when I first tried to get my head around it. Should the scheduling functionality in SnapPuller really move to ReplicationHandler?

        Show
        Ramkumar Aiyengar added a comment - +1 for the current patch, it does fix stuff, but in doFetch , shouldn't the finally block have the same cleanup? FWIW, this entire (temp)SnapPuller stuff is pretty messy, took me quite a bit of time when I first tried to get my head around it. Should the scheduling functionality in SnapPuller really move to ReplicationHandler ?
        Hide
        Ramkumar Aiyengar added a comment -

        I started taking a dig at this with https://github.com/bloomberg/lucene-solr/commit/50a198e518d66380e4bfef81baddd7fd27ffa198 A couple of tests fail though, I need to take a look.. With this refactoring in place, it looks like tempSnapPuller can go away, but may be I am missing something..

        Show
        Ramkumar Aiyengar added a comment - I started taking a dig at this with https://github.com/bloomberg/lucene-solr/commit/50a198e518d66380e4bfef81baddd7fd27ffa198 A couple of tests fail though, I need to take a look.. With this refactoring in place, it looks like tempSnapPuller can go away, but may be I am missing something..
        Hide
        Mark Miller added a comment -

        Thanks for the review.

        this entire (temp)SnapPuller stuff is pretty messy

        Agreed.

        Show
        Mark Miller added a comment - Thanks for the review. this entire (temp)SnapPuller stuff is pretty messy Agreed.
        Hide
        Mark Miller added a comment -

        Side Note from looking at your patch: We should rename SnapPuller to something more meaningful, like IndexFetcher.

        Show
        Mark Miller added a comment - Side Note from looking at your patch: We should rename SnapPuller to something more meaningful, like IndexFetcher.
        Hide
        Mark Miller added a comment -

        shouldn't the finally block have the same cleanup?

        I'm not sure - the tempSnapPuller is kept around for stats calls, and I'm worried about using it after destroy is called. Has a bad smell.

        Show
        Mark Miller added a comment - shouldn't the finally block have the same cleanup? I'm not sure - the tempSnapPuller is kept around for stats calls, and I'm worried about using it after destroy is called. Has a bad smell.
        Hide
        Ramkumar Aiyengar added a comment -

        I'm not sure - the tempSnapPuller is kept around for stats calls, and I'm worried about using it after destroy is called. Has a bad smell.

        Ah. Yeah, this entire stats business is actually the only thing which makes me nervous about removing temp as well. tempSnapPuller is assigned to snapPuller at the end, so you won't be calling it on a destroyed object, but I need to figure out what the intent of the stats is – is it for the last doFetch call done by replicate, or something else? If it's the former, what's snapPuller doing?

        Show
        Ramkumar Aiyengar added a comment - I'm not sure - the tempSnapPuller is kept around for stats calls, and I'm worried about using it after destroy is called. Has a bad smell. Ah. Yeah, this entire stats business is actually the only thing which makes me nervous about removing temp as well. tempSnapPuller is assigned to snapPuller at the end, so you won't be calling it on a destroyed object, but I need to figure out what the intent of the stats is – is it for the last doFetch call done by replicate, or something else? If it's the former, what's snapPuller doing?
        Hide
        Ramkumar Aiyengar added a comment -

        Raised SOLR-6804 for the untangling..

        Show
        Ramkumar Aiyengar added a comment - Raised SOLR-6804 for the untangling..
        Hide
        Mark Miller added a comment -

        shouldn't the finally block have the same cleanup?

        No, the temp close is handled finally in the close hook. If it is changed first, then we close the previous one. The temp puller is kept around for stat calls as mentioned above currently.

        While we might improve some of that, I think this is correct for the current code.

        Show
        Mark Miller added a comment - shouldn't the finally block have the same cleanup? No, the temp close is handled finally in the close hook. If it is changed first, then we close the previous one. The temp puller is kept around for stat calls as mentioned above currently. While we might improve some of that, I think this is correct for the current code.
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-6793: ReplicationHandler does not destroy all of it's created SnapPullers.

        Show
        ASF subversion and git services added a comment - Commit 1650764 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1650764 ] SOLR-6793 : ReplicationHandler does not destroy all of it's created SnapPullers.
        Hide
        ASF subversion and git services added a comment -

        Commit 1650765 from Mark Miller in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1650765 ]

        SOLR-6793: ReplicationHandler does not destroy all of it's created SnapPullers.

        Show
        ASF subversion and git services added a comment - Commit 1650765 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1650765 ] SOLR-6793 : ReplicationHandler does not destroy all of it's created SnapPullers.
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development