You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "patsonluk (via GitHub)" <gi...@apache.org> on 2024/07/30 23:45:58 UTC

[PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

patsonluk opened a new pull request, #2607:
URL: https://github.com/apache/solr/pull/2607

   https://issues.apache.org/jira/browse/SOLR-17386
   
   # Description
   It was reported that calling /admin/cores?action=RELOAD&core=<synthetic core on coordinator node> gives 500
   
   # Cause
   This is caused by the change introduced in https://github.com/apache/solr/pull/2438, which core from coordinator node is now of class `SyntheticSolrCore`, which is a child class of `SolrCore`, (instead of being `SolrCore`) and the dummy coordinator collection no longer registers against the zk. 
   
   There are 2 main issues arise because of this change:
   1. On a `reload` call of the coordinator core, since the implementation class `SyntheticSolrCore` does not override `reload`, it will run the `SolrCore` implementation which create a new `SolrCore` (instead of `SyntheticSorlCore`) as a replacement.
   2. In `CoreContainer#reload`, it will attempt to load the `DocCollection` from ZK (via cluster state) of the core's collection, and perform various logic (index writer, replication) base on the result. However, for synthetic solr core, such collection does not really exist.
   
   # Solution
   Corresponding to the causes:
   1. Overload the `reload` method in `SyntheticSolrCore` , it is a bit hard to just call `super.reload` as it will require quite a bit of refactoring. Instead, we will just use a simplified (ignore all the write/indexing handling) version of `reload`.
   2. Introduce a `SolrCore#isSynthetic` method, which return false for all cores except from `SyntheticSolrCore`. Then in `CoreContainer#reload` make a check on such method before getting the `DocCollection` from ZK. For `SyntheticSolrCore`, it will get null `DocCollection` which is already being handled in existing `CoreContainer` logic
   
   
   # Concerns
   I have concerns on the scope of change in this PR, in particular we will need to modify existing core classes `SolrCore` and `CoreContainer`. Both need to be somehow aware of the `Synthetic` core, which is really not ideal. Another option is to refactor the code to break things down and then override the corresponding parts in `SyntheticSolrCore`, this could be quite a bit of refactoring and might even change more of the existing core classes (but then they will not be "synthetic core" aware anymore!)
   
   Any thoughts please? @dsmiley @noblepaul  🙏🏼 
   
   # Tests
   
   Added `TestCoordinatorRole#testCoreReload` to perform config update then followed by a core reload. Ensure that core reload returns success response code and query updates according to the config update.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request title.
   - [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
   - [x] I have developed this patch against the `main` branch.
   - [x] I have run `./gradlew check`.
   - [x] I have added tests for my changes.
   - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2276526792

   AFAICT, the only remaining step is to _remove_ the CHANGES.txt entry, possibly update the existing one to include contributors.  I could do this or @cpoerschke .
   I'd love to see this bug fix in for 9.7!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1698878150


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {

Review Comment:
   Thank you for the suggestion @cpoerschke ! 😊  I don't quite know the preference in general in the Solr dev community - I worked in some teams that avoid `instanceof` whenever possible.
   
   I didn't ask for the full rationale behind that, my guess is probably due to the potential risk of missing future classes, which does not extend such target class yet inherit some similarity that fulfills the same condition. Having a `isXYZ` method could be slightly more flexible than forcing class hierarchy.
   
   Anyway, I'm pretty okay with either way - whatever fits better for Solr community standards. The obvious benefit of using `instanceof` in this case is to avoid the extra `isSynthetic` method. 💪🏼 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2264573626

   AFAIUI, This is a bug fix to a regression within 9.7, specifically to SOLR-17269.  No released version has this bug.  Therefore I don't think we need to list this in the bug fix section or perhaps not in CHANGES.txt at all (no user will care/know of a problem they have never seen!).  If any of you have your hearts set on a CHANGES.txt mention, maybe the existing SOLR-17269 could be "SOLR-17269, SOLR-17386 (it's okay to list multiple instead of just one) and add people to the attribution.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1703764178


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {
           docCollection = getZkController().getClusterState().getCollection(cd.getCollectionName());
           // turn off indexing now, before the new core is registered
           if (docCollection.getBool(ZkStateReader.READ_ONLY, false)) {

Review Comment:
   Thanks for the input! Let's go without the `isSynthetic` then.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2276570553

   Pushed 2 [changes](https://github.com/apache/solr/pull/2607/commits/aeadce89abe1f8c8239224161306a0c10055b559):
   1. Integrated [cpoerschke](https://github.com/cpoerschke)'s suggested change. We now removed the isSynthetic method and use `getCollectionOrNull` instead when checking the collection from zk in `CoreContainer`
   2. Merged the change entries in `CHANGE.txt`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2260007256

   > At least the change in CoreContainer & SolrCore here are about as trivial as can be. The complexity is in SyntheticSolrCore.reload().
   
   It seems (from a quick exploration) that factoring out a `newSolrCore` method could allow `SyntheticSolrCore` to override that method and then the `reload` method would not need to be overridden.
   
   ```
            cd.loadExtraProperties(); // Reload the extra properties
            core =
   -            new SolrCore(
   +            newSolrCore(
   
   +  protected SolrCore newSolrCore(
   +      CoreContainer coreContainer,
   +      CoreDescriptor coreDescriptor,
   +      ConfigSet configSet,
   +      String dataDir,
   +      UpdateHandler updateHandler,
   +      IndexDeletionPolicyWrapper delPolicy,
   +      SolrCore prev,
   +      boolean reload) {
   +    return new SolrCore(
   +        coreContainer, coreDescriptor, configSet, dataDir, updateHandler, delPolicy, prev, reload);
   +  }
   
   +  @Override
   +  protected SolrCore newSolrCore(
   +      CoreContainer coreContainer,
   +      CoreDescriptor coreDescriptor,
   +      ConfigSet configSet,
   +      String dataDir,
   +      UpdateHandler updateHandler,
   +      IndexDeletionPolicyWrapper delPolicy,
   +      SolrCore prev,
   +      boolean reload) {
   +    return new SyntheticSolrCore(
   +        coreContainer, coreDescriptor, configSet, dataDir, updateHandler, delPolicy, prev, reload);
   +  }
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1698868734


##########
solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java:
##########
@@ -72,4 +88,43 @@ protected RestManager initRestManager() throws SolrException {
     // We do not expect RestManager ops on Coordinator Nodes
     return new RestManager();
   }
+
+  @Override
+  public SolrCore reload(ConfigSet coreConfig) throws IOException {
+    // only one reload at a time
+    synchronized (getUpdateHandler().getSolrCoreState().getReloadLock()) {
+      solrCoreState.increfSolrCoreState();
+      boolean success = false;
+      SyntheticSolrCore newCore = null;
+      try {
+        CoreDescriptor newCoreDescriptor = new CoreDescriptor(getName(), getCoreDescriptor());
+        newCoreDescriptor.loadExtraProperties(); // Reload the extra properties
+
+        newCore =
+            new SyntheticSolrCore(
+                getCoreContainer(),
+                newCoreDescriptor,
+                coreConfig,
+                getDataDir(),
+                getUpdateHandler(),
+                getDeletionPolicy(),
+                this,
+                true);
+
+        newCore.getSearcher(true, false, null, true);
+        success = true;
+        return newCore;
+      } finally {
+        // close the new core on any errors that have occurred.
+        if (!success && newCore != null && newCore.getOpenCount() > 0) {

Review Comment:
   This part is copied from `SolrCore#reload`, I don't know the code super well so I'm just making guesses here 😅  - This makes sure whatever resource allocated by a "failed" core to be released before leaving this method. I think for normal successful case, it should not close the new core returned.
   
   `Maybe it's better to switch them directly instead of returning and jumping back to close?` are you referring to the fact that a non null core can be returned yet it's getting closed here if it's a bad one? Honestly I don't understand the full rational behind the comment `// close the new core on any errors that have occurred.` neither 🤔 . Though it's probably the safest to just follow suit in case this is needed to prevent some unexpected resource leak perhaps.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "ellaeln (via GitHub)" <gi...@apache.org>.
ellaeln commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2262794835

   ellaeln is good 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1698101054


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {

Review Comment:
   maybe
   ```suggestion
           if (getZkController() != null && !(newCore instanceof SyntheticSolrCore)) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2261198296

   > new SyntheticSolrCore(
   > +        coreContainer, coreDescriptor, configSet, dataDir, updateHandler, delPolicy, prev, reload);
   
   That's a brilliant suggestion! ✨ 
   
   I will push a change soon with slight modification (using a different method name and signature to make the usage a bit more specific to our case 😊 )


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke merged PR #2607:
URL: https://github.com/apache/solr/pull/2607


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1700444070


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {
           docCollection = getZkController().getClusterState().getCollection(cd.getCollectionName());
           // turn off indexing now, before the new core is registered
           if (docCollection.getBool(ZkStateReader.READ_ONLY, false)) {

Review Comment:
   I do like your suggestion as we can avoid introduction of `isSynthetic` and SolrCore/CoreContainer can remain unaware of the synthetic collection, which is a major ➕ ! 
   
   Perhaps we can get some input from @dsmiley on this as well? 😊  🙏🏼 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1701823507


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {
           docCollection = getZkController().getClusterState().getCollection(cd.getCollectionName());
           // turn off indexing now, before the new core is registered
           if (docCollection.getBool(ZkStateReader.READ_ONLY, false)) {

Review Comment:
   I'm good with this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "ellaeln (via GitHub)" <gi...@apache.org>.
ellaeln commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2265114996

   I tested things again and everything looks OK. Is there anything else that needs to get done before we can move this forward?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1699960636


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2056,7 +2056,7 @@ public void reload(String name, UUID coreId) {
         newCore = core.reload(coreConfig);
 
         DocCollection docCollection = null;
-        if (getZkController() != null) {
+        if (getZkController() != null && !newCore.isSynthetic()) {
           docCollection = getZkController().getClusterState().getCollection(cd.getCollectionName());
           // turn off indexing now, before the new core is registered
           if (docCollection.getBool(ZkStateReader.READ_ONLY, false)) {

Review Comment:
   perhaps this would be a little controversial since it's modifying existing behaviour for non-synthetic cores also, albeit error logic code path
   ```suggestion
             docCollection = getZkController().getClusterState().getCollectionOrNull(cd.getCollectionName());
             // turn off indexing now, before the new core is registered
             if (docCollection != null && docCollection.getBool(ZkStateReader.READ_ONLY, false)) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2263450355

   > Planning ahead to PR merge time -- `solr/CHANGES.txt` -- @patsonluk please could you add an entry in the Solr 9.7 bug fix section, e.g. something like this?
   > 
   > ```
   > SOLR-17386: Fix coordinators (SyntheticSolrCore) reload issue. (ellaeln, Patson Luk, David Smiley, Christine Poerschke)
   > ```
   > 
   > (GitHub won't allow me to directly edit it on the PR's branch here, likely the known issue due to repo fork being an org level fork, just FYI.)
   > 
   > @ellaeln - please let us know what your preferred attribution in `solr/CHANGES.txt` is, some people go with handle, others first name last name, it's up to you. Thanks!
   
   @cpoerschke  Updated CHANGES.txt as proposed. Many thanks for pushing this through! 🙇🏼 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2262799967

   > ... entry in the Solr 9.7 bug fix section ...
   
   Solr 9.7 release dev list thread: https://lists.apache.org/thread/trhhb1q84gjzz9z9jx931m2p7v3q6wdn


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "ellaeln (via GitHub)" <gi...@apache.org>.
ellaeln commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2259919632

   thanks for this fix, it's very important for us. I'll play with it a bit to see if there's any potential issue i can spot


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1699048655


##########
solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java:
##########
@@ -72,4 +88,43 @@ protected RestManager initRestManager() throws SolrException {
     // We do not expect RestManager ops on Coordinator Nodes
     return new RestManager();
   }
+
+  @Override
+  public SolrCore reload(ConfigSet coreConfig) throws IOException {
+    // only one reload at a time
+    synchronized (getUpdateHandler().getSolrCoreState().getReloadLock()) {
+      solrCoreState.increfSolrCoreState();
+      boolean success = false;
+      SyntheticSolrCore newCore = null;
+      try {
+        CoreDescriptor newCoreDescriptor = new CoreDescriptor(getName(), getCoreDescriptor());
+        newCoreDescriptor.loadExtraProperties(); // Reload the extra properties
+
+        newCore =
+            new SyntheticSolrCore(
+                getCoreContainer(),
+                newCoreDescriptor,
+                coreConfig,
+                getDataDir(),
+                getUpdateHandler(),
+                getDeletionPolicy(),
+                this,
+                true);
+
+        newCore.getSearcher(true, false, null, true);
+        success = true;
+        return newCore;
+      } finally {
+        // close the new core on any errors that have occurred.
+        if (!success && newCore != null && newCore.getOpenCount() > 0) {

Review Comment:
   After taking the great suggestion from @cpoerschke , we no longer need to overwrite the whole reload method 😊  



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2262786338

   Planning ahead to PR merge time -- `solr/CHANGES.txt` -- @patsonluk please could you add an entry in the Solr 9.7 bug fix section, e.g. something like this?
   
   ```
   SOLR-17386: Fix coordinators (SyntheticSolrCore) reload issue. (ellaeln, Patson Luk, David Smiley, Christine Poerschke)
   ```
   
   (GitHub won't allow me to directly edit it on the PR's branch here, likely the known issue due to repo fork being an org level fork, just FYI.)
   
   @ellaeln - please let us know what your preferred attribution in `solr/CHANGES.txt` is, some people go with handle, others first name last name, it's up to you. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "ellaeln (via GitHub)" <gi...@apache.org>.
ellaeln commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1698263165


##########
solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java:
##########
@@ -72,4 +88,43 @@ protected RestManager initRestManager() throws SolrException {
     // We do not expect RestManager ops on Coordinator Nodes
     return new RestManager();
   }
+
+  @Override
+  public SolrCore reload(ConfigSet coreConfig) throws IOException {
+    // only one reload at a time
+    synchronized (getUpdateHandler().getSolrCoreState().getReloadLock()) {
+      solrCoreState.increfSolrCoreState();
+      boolean success = false;
+      SyntheticSolrCore newCore = null;
+      try {
+        CoreDescriptor newCoreDescriptor = new CoreDescriptor(getName(), getCoreDescriptor());
+        newCoreDescriptor.loadExtraProperties(); // Reload the extra properties
+
+        newCore =
+            new SyntheticSolrCore(
+                getCoreContainer(),
+                newCoreDescriptor,
+                coreConfig,
+                getDataDir(),
+                getUpdateHandler(),
+                getDeletionPolicy(),
+                this,
+                true);
+
+        newCore.getSearcher(true, false, null, true);
+        success = true;
+        return newCore;
+      } finally {
+        // close the new core on any errors that have occurred.
+        if (!success && newCore != null && newCore.getOpenCount() > 0) {

Review Comment:
   This is a bit weird because the act of returning the new core causes the newCoreCount to be bigger than 0 and triggers the close. Maybe it's better to switch them directly instead of returning and jumping back to close?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1699239949


##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -805,6 +792,24 @@ public SolrCore reload(ConfigSet coreConfig) throws IOException {
     }
   }
 
+  /**
+   * Clones the current core for core reload, with the provided CoreDescriptor and ConfigSet.
+   *
+   * @return the cloned core to be used for SolrCore#load
+   */
+  protected SolrCore cloneAsReloadCore(

Review Comment:
   the name of this method reads weird to me... maybe cloneForReloadedCore sounds better



##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -805,6 +792,24 @@ public SolrCore reload(ConfigSet coreConfig) throws IOException {
     }
   }
 
+  /**
+   * Clones the current core for core reload, with the provided CoreDescriptor and ConfigSet.
+   *
+   * @return the cloned core to be used for SolrCore#load

Review Comment:
   Not for SolrCore#reload but SolrCore#reload -- you could actually link to the method instead BTW



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2276546631

   Sorry guys. I must have missed the notifications! Lemme summarize all the changes and make one final commit !


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

Re: [PR] SOLR-17386: Fixed SyntheticSolrCore reload issue [solr]

Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #2607:
URL: https://github.com/apache/solr/pull/2607#issuecomment-2277500262

   > Pushed 2 [changes](https://github.com/apache/solr/pull/2607/commits/aeadce89abe1f8c8239224161306a0c10055b559):
   > 
   > 1. Integrated [cpoerschke](https://github.com/cpoerschke)'s suggested change. We now removed the isSynthetic method and use `getCollectionOrNull` instead when checking the collection from zk in `CoreContainer`
   > 2. Merged the change entries in `CHANGE.txt`
   
   Thanks! Merged and backported to `branch_9x` and `branch_9_7` branches.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org