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

Deprecate ZkStateReader.updateClusterState(), remove uses

    Details

    • Flags:
      Patch

      Description

      Forcing a full ZK cluster state update creates a lot of unnecessary work and load at scale. We need to deprecate and remove existing callers.

      • The one at the start of ClusterStateUpdater thread is fine, it's a one-time thing.
      • The one in OverseerCollectionMessageHandler is getting removed in SOLR-8722
      • The rest of them can be replaced with a version that only updates a single collection; not everything!

      Patch will be forthcoming.

      1. SOLR-8745.patch
        63 kB
        Shalin Shekhar Mangar
      2. SOLR-8745.patch
        61 kB
        Scott Blum

        Issue Links

          Activity

          Hide
          dragonsinth Scott Blum added a comment -

          Patch!

          Show
          dragonsinth Scott Blum added a comment - Patch!
          Hide
          dragonsinth Scott Blum added a comment -
          Show
          dragonsinth Scott Blum added a comment - Shalin Shekhar Mangar
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Instead of zkStateReader.forceUpdateCollection(collection), can we overload the updateClusterState method itself with the collection name as a parameter, e.g. zkStateReader.updateClusterState(collection)? My thought is that forceUpdateCollection gives a sense of a forceful update happening to a collection, which is not the case here.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Instead of zkStateReader.forceUpdateCollection(collection) , can we overload the updateClusterState method itself with the collection name as a parameter, e.g. zkStateReader.updateClusterState(collection) ? My thought is that forceUpdateCollection gives a sense of a forceful update happening to a collection, which is not the case here.
          Hide
          dragonsinth Scott Blum added a comment -

          I specifically added "force" with the intent of disincentivizing callers. The correct pattern is to loop until cluster state reflects the state you expect to see.

          Show
          dragonsinth Scott Blum added a comment - I specifically added "force" with the intent of disincentivizing callers. The correct pattern is to loop until cluster state reflects the state you expect to see.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          In such a case, do you think forceUpdateClusterState(collection) is a better choice than forceUpdateCollection(collection)?
          Maybe, we could rename the existing method, updateClusterState(), to forceUpdateClusterState() as well?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - In such a case, do you think forceUpdateClusterState(collection) is a better choice than forceUpdateCollection(collection)? Maybe, we could rename the existing method, updateClusterState(), to forceUpdateClusterState() as well?
          Hide
          dragonsinth Scott Blum added a comment -

          Good call, I like forceUpdateCollection(). I don't see a strong need to rename the original method since we want to get rid of it.

          Show
          dragonsinth Scott Blum added a comment - Good call, I like forceUpdateCollection(). I don't see a strong need to rename the original method since we want to get rid of it.
          Hide
          romseygeek Alan Woodward added a comment -

          This ties in nicely with SOLR-8323 as well. In fact, we can probably replace the vast majority of calls to updateClusterState() with calls to waitForState(), and maybe make the method private.

          Show
          romseygeek Alan Woodward added a comment - This ties in nicely with SOLR-8323 as well. In fact, we can probably replace the vast majority of calls to updateClusterState() with calls to waitForState(), and maybe make the method private.
          Hide
          dragonsinth Scott Blum added a comment -

          BTW: it's on my TODO list to take a look at SOLR-8323!

          Show
          dragonsinth Scott Blum added a comment - BTW: it's on my TODO list to take a look at SOLR-8323 !
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          ZkStateReader.forceUpdateCollection has the following:

          else if (ref.isLazilyLoaded()) {
                  if (ref.get() != null) {
                    return;
                  }
                  // Edge case: if there's no external collection, try refreshing legacy cluster state in case it's there.
                  refreshLegacyClusterState(null);
                }
          

          Scott, can you please explain when this edge case may be triggered?

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - ZkStateReader.forceUpdateCollection has the following: else if (ref.isLazilyLoaded()) { if (ref.get() != null ) { return ; } // Edge case : if there's no external collection, try refreshing legacy cluster state in case it's there. refreshLegacyClusterState( null ); } Scott, can you please explain when this edge case may be triggered?
          Hide
          dragonsinth Scott Blum added a comment -

          Sure! This was necessary to get tests to pass. Lazy collection refs get created based on the children of /solr/collections, regardless of whether a state.json exists for that collection. So e.g. an empty /solr/collections/FOO will cause a lazy FOO collection to be created. If there's no state.json, ref.get() will return null. However, as an edge case, someone could have just added FOO to clusterstate.json as a stateformat 1 collection, and so we can refresh the legacy state to check.

          Show
          dragonsinth Scott Blum added a comment - Sure! This was necessary to get tests to pass. Lazy collection refs get created based on the children of /solr/collections, regardless of whether a state.json exists for that collection. So e.g. an empty /solr/collections/FOO will cause a lazy FOO collection to be created. If there's no state.json, ref.get() will return null. However, as an edge case, someone could have just added FOO to clusterstate.json as a stateformat 1 collection, and so we can refresh the legacy state to check.
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Thanks for explaining.

          This patch fixes a test failure in TestMiniSolrCloudCluster which was creating a ZkStateReader and attempting to get cluster state without either calling createClusterStateWatchers or updateClusterState. This test doesn't need to create a new ZkStateReader. I fixed it to use the ZkStateReader from the CloudSolrClient.

          All tests and precommit checks pass. This is ready.

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Thanks for explaining. This patch fixes a test failure in TestMiniSolrCloudCluster which was creating a ZkStateReader and attempting to get cluster state without either calling createClusterStateWatchers or updateClusterState. This test doesn't need to create a new ZkStateReader. I fixed it to use the ZkStateReader from the CloudSolrClient. All tests and precommit checks pass. This is ready.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 093a8ce57c06f1bf2f71ddde52dcc7b40cbd6197 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=093a8ce ]

          SOLR-8745: Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow forceUpdateCollection(collection)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 093a8ce57c06f1bf2f71ddde52dcc7b40cbd6197 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=093a8ce ] SOLR-8745 : Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow forceUpdateCollection(collection)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit dd04b6173955d55348b3abaec4c2a3e875e12487 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dd04b61 ]

          SOLR-8745: Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow forceUpdateCollection(collection)
          (cherry picked from commit 093a8ce)

          Show
          jira-bot ASF subversion and git services added a comment - Commit dd04b6173955d55348b3abaec4c2a3e875e12487 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dd04b61 ] SOLR-8745 : Deprecate costly ZkStateReader.updateClusterState(), replace with a narrow forceUpdateCollection(collection) (cherry picked from commit 093a8ce)
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Thanks Scott!

          SOLR-8323 is very interesting and I intend to take a deeper look into it, thanks for the ping Alan.

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Thanks Scott! SOLR-8323 is very interesting and I intend to take a deeper look into it, thanks for the ping Alan.
          Hide
          dragonsinth Scott Blum added a comment -

          woot!

          Show
          dragonsinth Scott Blum added a comment - woot!
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ]

          SOLR-8745: Move CHANGES.txt entry to 6.1

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ] SOLR-8745 : Move CHANGES.txt entry to 6.1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d968575b8b6035c58de3c0f204dde27611c535b2 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d968575 ]

          SOLR-8745: Move CHANGES.txt entry to 6.1
          (cherry picked from commit 4fbfeb0)

          Show
          jira-bot ASF subversion and git services added a comment - Commit d968575b8b6035c58de3c0f204dde27611c535b2 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d968575 ] SOLR-8745 : Move CHANGES.txt entry to 6.1 (cherry picked from commit 4fbfeb0)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/apiv2 from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ]

          SOLR-8745: Move CHANGES.txt entry to 6.1

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/apiv2 from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ] SOLR-8745 : Move CHANGES.txt entry to 6.1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/jira/SOLR-445 from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ]

          SOLR-8745: Move CHANGES.txt entry to 6.1

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4fbfeb01230429b073039b4d16b8871c1854f413 in lucene-solr's branch refs/heads/jira/ SOLR-445 from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4fbfeb0 ] SOLR-8745 : Move CHANGES.txt entry to 6.1
          Hide
          hossman Hoss Man added a comment -

          Manually correcting fixVersion per Step #S5 of LUCENE-7271

          Show
          hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b663e5bcad9974f2d80c16b85c862407a38290e0 in lucene-solr's branch refs/heads/branch_6_0 from Scott Blum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b663e5b ]

          SOLR-8973: Zookeeper frenzy when a core is first created.

          For branch_6_0: Modified ZkStateReaderTest to use ZkStateReader.updateClusterState() instead of .forceUpdateCollection(), since SOLR-8745 will land in 6.1.

          Show
          jira-bot ASF subversion and git services added a comment - Commit b663e5bcad9974f2d80c16b85c862407a38290e0 in lucene-solr's branch refs/heads/branch_6_0 from Scott Blum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b663e5b ] SOLR-8973 : Zookeeper frenzy when a core is first created. For branch_6_0: Modified ZkStateReaderTest to use ZkStateReader.updateClusterState() instead of .forceUpdateCollection(), since SOLR-8745 will land in 6.1.

            People

            • Assignee:
              shalinmangar Shalin Shekhar Mangar
              Reporter:
              dragonsinth Scott Blum
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development