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

Clean up references to CoreContainer and CoreDescriptors

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.4
    • Fix Version/s: 6.6, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      It's a bit weird that CoreDescriptor contains a reference to CoreContainer. It seems like the SolrCore should reference CoreContainer.

      Similarly, SolrCore keeps a copy of CoreDescriptor and another copy is kept in the various CoreDescirptor lists making it difficult to track which is the "real" CoreDescriptor.

      This is an umbrella issue as I think this (and perhaps other) issues can be tackled separately.

      1. SOLR-10007.patch
        123 kB
        Erick Erickson
      2. SOLR-10007.patch
        112 kB
        Erick Erickson
      3. SOLR-10007.patch
        111 kB
        Erick Erickson
      4. SOLR-10007.patch
        109 kB
        Erick Erickson

        Issue Links

          Activity

          Hide
          erickerickson Erick Erickson added a comment -

          Here's what this patch is looking like. Something of a WIP, but reasonably close I think.

          It touches a lot of files. Mostly it's just something akin to:

          • this(core.getCoreDescriptor().getCoreContainer(), core.getName());
            + this(core.getCoreContainer(), core.getName());

          I seem to be having trouble with, of all things, TestConfigSetsAPI. Haven't run precommit either.

          Show
          erickerickson Erick Erickson added a comment - Here's what this patch is looking like. Something of a WIP, but reasonably close I think. It touches a lot of files. Mostly it's just something akin to: this(core.getCoreDescriptor().getCoreContainer(), core.getName()); + this(core.getCoreContainer(), core.getName()); I seem to be having trouble with, of all things, TestConfigSetsAPI. Haven't run precommit either.
          Hide
          erickerickson Erick Erickson added a comment -

          I think this is very close to ready, running tests and precommit and such again.

          It touches a lot of files, but most of them are changing solrcore.getCoreDescriptor().getCoreContainer()
          to
          solrcore.getCoreContainer().

          Along the way I renamed a few internal variables to more accurately reflect what they're supposed to be.

          There may be a few gratuitous format changes, I upgraded IntelliJ and somehow had the "strip trailing spaces on save" set and "reformat whole file" rather than "only reformat VCS changed text". I went through and undid all the gratuitous junk I could find, but a few might have snuck through.

          Structurally this is significantly better I think. There were a number of places where we were using the CoreDescriptors to infer the state of the SolrCore. The big thing this patch does is try to separate the use of CoreDescriptors and SolrCores.

          The other change here is it moves the CoreContainer reference from the CoreDescriptor to SolrCore.

          I'll commit this in a day or two unless there are objections. It' incorporates both SOLR-10008 and SOLR-10009 so I'll close those now.

          Show
          erickerickson Erick Erickson added a comment - I think this is very close to ready, running tests and precommit and such again. It touches a lot of files, but most of them are changing solrcore.getCoreDescriptor().getCoreContainer() to solrcore.getCoreContainer(). Along the way I renamed a few internal variables to more accurately reflect what they're supposed to be. There may be a few gratuitous format changes, I upgraded IntelliJ and somehow had the "strip trailing spaces on save" set and "reformat whole file" rather than "only reformat VCS changed text". I went through and undid all the gratuitous junk I could find, but a few might have snuck through. Structurally this is significantly better I think. There were a number of places where we were using the CoreDescriptors to infer the state of the SolrCore. The big thing this patch does is try to separate the use of CoreDescriptors and SolrCores. The other change here is it moves the CoreContainer reference from the CoreDescriptor to SolrCore. I'll commit this in a day or two unless there are objections. It' incorporates both SOLR-10008 and SOLR-10009 so I'll close those now.
          Hide
          erickerickson Erick Erickson added a comment -

          First time I've ever had precompile fail but still be able to 'ant clean server dist' successfully due to a renamed method.

          Patch now passes precommit and test.

          Show
          erickerickson Erick Erickson added a comment - First time I've ever had precompile fail but still be able to 'ant clean server dist' successfully due to a renamed method. Patch now passes precommit and test.
          Hide
          erickerickson Erick Erickson added a comment -

          Final patch with CHANGES entry.

          I've tested this pretty thoroughly, all tests pass 20 iterations of the full unit tests.

          This is a big patch, but it has a lot of mechanical changes, i.e. core.getCoreDescriptor().getCoreContainer()
          is now just
          core.getCoreContainer()

          Significant fragility in the code was because we kept CoreDescriptors in multiple places, which is no longer the case.

          I also renamed a couple of the internal methods to make their purpose clearer.

          I also added a "reload" method to CoresLocator, there has to be the possibility of re-reading the coreDescriptor from disk.

          Show
          erickerickson Erick Erickson added a comment - Final patch with CHANGES entry. I've tested this pretty thoroughly, all tests pass 20 iterations of the full unit tests. This is a big patch, but it has a lot of mechanical changes, i.e. core.getCoreDescriptor().getCoreContainer() is now just core.getCoreContainer() Significant fragility in the code was because we kept CoreDescriptors in multiple places, which is no longer the case. I also renamed a couple of the internal methods to make their purpose clearer. I also added a "reload" method to CoresLocator, there has to be the possibility of re-reading the coreDescriptor from disk.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4cb00ccca40854f4b51ea7edb829d48228c95673 in lucene-solr's branch refs/heads/master from Erick Erickson
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4cb00cc ]

          SOLR-10007: Clean up references to CoreContainer and CoreDescriptors

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4cb00ccca40854f4b51ea7edb829d48228c95673 in lucene-solr's branch refs/heads/master from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4cb00cc ] SOLR-10007 : Clean up references to CoreContainer and CoreDescriptors
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit acf302202a15426eee148f58346689ff42dd23e4 in lucene-solr's branch refs/heads/branch_6x from Erick Erickson
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=acf3022 ]

          SOLR-10007 Clean up references to CoreContainer and CoreDescriptors

          Show
          jira-bot ASF subversion and git services added a comment - Commit acf302202a15426eee148f58346689ff42dd23e4 in lucene-solr's branch refs/heads/branch_6x from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=acf3022 ] SOLR-10007 Clean up references to CoreContainer and CoreDescriptors

            People

            • Assignee:
              erickerickson Erick Erickson
              Reporter:
              erickerickson Erick Erickson
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development