Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8633

DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5.1, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      trying to wrap my head around a weird bug in my experiements with SOLR-445, i realized that DUP.processDelete has a direct call to finish().

      This violates the normal lifecycle of an UpdateProcessor (finish is only suppose to be called exactly once after processing any/all UpdateCommands) and could potentially break any UpdateProcessors configured after DUP (or in my case: processors configured before DUP that expect to be in charge of calling finish, and catching any resulting exceptions, as part of the normal life cycle)

      Independent of how it impacts other update processors, this also means that:

      1. all the logic in DUP.doFinish is getting executed twice – which seems kind of expensive/dangerous to me since there is leader initiated recovery involved in this method
      2. SolrCmdDistributor.finish() gets called twice, which means StreamingSolrClients.shutdown() gets called twice, which means ConcurrentUpdateSolrClient.close() gets called twice ... it seems like we're just getting really lucky that (as configured by DUP) all of these resources are still usable after being finished/shutdown/closed

        Issue Links

          Activity

          Hide
          hossman Hoss Man added a comment -

          I suspect that the intended goal here (in processCommit) is to do the same sort of "force sequential ordering" type logic as doDeleteByQuery – in which case we can/should replace the call to finish() with a call to cmdDistrib.finish() ... except that i'm still concerned about shutdown/closing of clients done by that method, and wonder if both processCommit & processDelete should be calling SolrCmdDistributor.blockAndDoRetries() directly? (we would just need to make that method public)

          Hoping folks like Mark Miller / Timothy Potter might have some ideas about this?

          Show
          hossman Hoss Man added a comment - I suspect that the intended goal here (in processCommit) is to do the same sort of "force sequential ordering" type logic as doDeleteByQuery – in which case we can/should replace the call to finish() with a call to cmdDistrib.finish() ... except that i'm still concerned about shutdown/closing of clients done by that method, and wonder if both processCommit & processDelete should be calling SolrCmdDistributor.blockAndDoRetries() directly? (we would just need to make that method public) Hoping folks like Mark Miller / Timothy Potter might have some ideas about this?
          Hide
          hossman Hoss Man added a comment -

          attaching a strawman patch that has asserts ensuring that finish() is never called more then once on either DUP or SolrCmdDistributor, and makes blockAndDoRetries() public so DUP can call it instead of finish()

          all tests pass ... anyone see any problems with this?

          any reason why the previous code (where all the logic in DUP.finish() was getting executed twice when there was an explicit commit) was better/needed ?

          Show
          hossman Hoss Man added a comment - attaching a strawman patch that has asserts ensuring that finish() is never called more then once on either DUP or SolrCmdDistributor, and makes blockAndDoRetries() public so DUP can call it instead of finish() all tests pass ... anyone see any problems with this? any reason why the previous code (where all the logic in DUP.finish() was getting executed twice when there was an explicit commit) was better/needed ?
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Healing. Happy to review soon (this week).

          Show
          markrmiller@gmail.com Mark Miller added a comment - Healing. Happy to review soon (this week).
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Forgot this issue. I'll review this today.

          Show
          markrmiller@gmail.com Mark Miller added a comment - Forgot this issue. I'll review this today.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Yeah, it's a bug, patch is the right fix.

          Show
          markrmiller@gmail.com Mark Miller added a comment - Yeah, it's a bug, patch is the right fix.
          Hide
          hossman Hoss Man added a comment -

          thanks Mark Miller - if you want to take this go ahead, otherwise i'll commit & backport first thing next week.

          Show
          hossman Hoss Man added a comment - thanks Mark Miller - if you want to take this go ahead, otherwise i'll commit & backport first thing next week.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8cd53a076b579ebc3be1fbb26875321e66a41608 in lucene-solr's branch refs/heads/master from markrmiller
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8cd53a0 ]

          SOLR-8633: DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8cd53a076b579ebc3be1fbb26875321e66a41608 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8cd53a0 ] SOLR-8633 : DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.
          Hide
          anshumg Anshum Gupta added a comment -

          Reopening for 5.5.1

          Show
          anshumg Anshum Gupta added a comment - Reopening for 5.5.1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 30fee6ba03fc77c51ff0e87a405f5b2942d8e7c9 in lucene-solr's branch refs/heads/branch_5x from markrmiller
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=30fee6b ]

          SOLR-8633: DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 30fee6ba03fc77c51ff0e87a405f5b2942d8e7c9 in lucene-solr's branch refs/heads/branch_5x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=30fee6b ] SOLR-8633 : DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e603565d4757625cc90d9db77a7ee240c20f93ed in lucene-solr's branch refs/heads/branch_5_5 from markrmiller
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e603565 ]

          SOLR-8633: DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.

          Show
          jira-bot ASF subversion and git services added a comment - Commit e603565d4757625cc90d9db77a7ee240c20f93ed in lucene-solr's branch refs/heads/branch_5_5 from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e603565 ] SOLR-8633 : DistributedUpdateProcess processCommit/deleteByQuery call finish on DUP and SolrCmdDistributor, which violates the lifecycle and can cause bugs.

            People

            • Assignee:
              markrmiller@gmail.com Mark Miller
              Reporter:
              hossman Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development