You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/07/16 04:39:20 UTC

[GitHub] [lucene-solr] dsmiley opened a new pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

dsmiley opened a new pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675


   https://issues.apache.org/jira/browse/SOLR-14652
   (minor refactoring)
   Also:
   * SolrCore's constructors don't need a "name" since it's guaranteed to always be the name in the coreDescriptor.  I checked.
   * SolrCore's constructor shouldn't call coreContainer.solrCores.addCoreDescriptor(cd); because it's the container's responsibility to manage such things.  I made SolrCores.putCore ensure the descriptor is added, and this is called by CoreContainer.registerCore which is called after new SolrCore instances are created.
   * solrCore.setName should only be called when we expect the name to change.  Furthermore that shouldn't ever happen in SolrCloud so I added checks.
   * solrCore.setName calls coreMetricManager.afterCoreSetName() which is something that is really only related to a rename, not name initialization (from the constructor).  I renamed that method and further only call it if the name did change from non-null.  CC @sigram 
   
   @ErickErickson you might want to do a code review based on your past interactions with the CoreContainer/CoreDescriptor/SolrCores/SolrCore relationship.
   
   I'm doubtful this deserves a CHANGES.txt entry; invisible to users and is relatively minor.


----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r457845217



##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -487,10 +488,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);
+    assert !renamed || coreDescriptor.getCloudDescriptor() == null : "Cores are not renamed in SolrCloud";

Review comment:
       I'll file a new issue for that.




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r460366284



##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1788,6 +1786,7 @@ public void unload(String name, boolean deleteIndexDir, boolean deleteDataDir, b
   }
 
   public void rename(String name, String toName) {
+    apiAssumeStandalone();

Review comment:
       I think it'd be more pertinent to update the Solr Ref Guide, which is what users would see.  This method here is very internal.  At least if a user tries it, it'll now not work with an error telling them so, as opposed to before, bad things would probably happen.




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley merged pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675


   


----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley commented on pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#issuecomment-661869359


   > Please give me until EOW to review this. Appreciate the patience, thanks!
   
   Sure.  You got me just in time :-)


----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r456530567



##########
File path: solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java
##########
@@ -92,10 +92,10 @@ public void loadReporters() {
    * are carried over and will be used under the new core name.
    * This method also reloads reporters so that they use the new core name.
    */
-  public void afterCoreSetName() {
+  public void afterCoreRename() {
+    assert core.getCoreDescriptor().getCloudDescriptor() == null;

Review comment:
       Should it call apiAssumeStandalone()?

##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -487,10 +488,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);
+    assert !renamed || coreDescriptor.getCloudDescriptor() == null : "Cores are not renamed in SolrCloud";

Review comment:
       Should it call apiAssumeStandalone()?




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r456684252



##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -487,10 +488,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);
+    assert !renamed || coreDescriptor.getCloudDescriptor() == null : "Cores are not renamed in SolrCloud";

Review comment:
       I chose to name "apiAssumeStandalone" that because it's intended for primary service/API methods (likely coming from a user) yielding a BAD_REQUEST instead of SERVER_ERROR.  SolrCore.setName is more of an internal detail method where an assertion seemed best.  Perhaps I should add a method on SolrCore "isSolrCloud" or "isStandalone" and/or assumeStandalone or assumeSolrCloud and use these more pervasively?  I feel that could be its own issue since there are many places that could be enhanced/clarified to use this.  Across Solr there are various techniques like checking the subclass of the core's SolrResourceLoader or checking if the CoreContainer has a ZkController.  A consistent approach would be nicer; perhaps separate for CoreContainer level vs SolrCore level




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] madrob commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r460288613



##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -497,10 +498,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);

Review comment:
       More clear as `!v.equals(this.name)` I think?




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] juanka588 commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
juanka588 commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r458117507



##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1788,6 +1786,7 @@ public void unload(String name, boolean deleteIndexDir, boolean deleteDataDir, b
   }
 
   public void rename(String name, String toName) {
+    apiAssumeStandalone();

Review comment:
       change java doc reflecting this change




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] madrob commented on pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#issuecomment-661866720


   Please give me until EOW to review this. Appreciate the patience, 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.

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



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

[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r457174905



##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -487,10 +488,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);
+    assert !renamed || coreDescriptor.getCloudDescriptor() == null : "Cores are not renamed in SolrCloud";

Review comment:
       +1 to have a consistent and more pervasive approach. It would first make the code clearer to use, and would probably avoid bugs.




----------------------------------------------------------------
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.

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



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

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1675: SOLR-14652: SolrCore should hold its own CoreDescriptor

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1675:
URL: https://github.com/apache/lucene-solr/pull/1675#discussion_r460365884



##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -497,10 +498,13 @@ public String getName() {
   }
 
   public void setName(String v) {
+    Objects.requireNonNull(v);
+    boolean renamed = this.name != null && !this.name.equals(v);

Review comment:
       I must check if this.name != null because I don't want to consider an initial population (from constructor) a _rename_ even though in some sense one could argue it is.
   
   I'll follow up this PR with removal of the existence of `logid` which will have the effect of simplifying setName which will in turn make it clearer to have the constructor simply set the name without calling _setName_.  At that point I might also rename setName to rename and ensure it's only called to actually do a rename.   Right now the constructor deserves to be calling setName because it populates `logid` and I wouldn't want that logic duplicated.




----------------------------------------------------------------
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.

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



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