[GitHub] [solr] noblepaul commented on a diff in pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset

["noblepaul (via GitHub)" <gi...@apache.org>]

noblepaul commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088500824


##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -178,8 +179,12 @@ private class ResourceProvider implements Function<String, InputStream> {
         ZkSolrResourceLoader.ZkByteArrayInputStream zkin =
             (ZkSolrResourceLoader.ZkByteArrayInputStream) in;
         zkVersion = zkin.getStat().getVersion();
-        hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getZnodeVersion());
+        hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getVersion());

Review Comment:
   why is `zkin.getStat().getCtime()` used to compute hash? isn't `zkVersion` not enough?



##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1136,10 +1137,23 @@ private SolrCore(
       updateProcessorChains = loadUpdateProcessorChains();
       reqHandlers = new RequestHandlers(this);
       reqHandlers.initHandlersFromConfig(solrConfig);
-      jerseyAppHandler =
-          (V2ApiUtils.isEnabled())
-              ? new ApplicationHandler(reqHandlers.getRequestHandlers().getJerseyEndpoints())
-              : null;
+      if (V2ApiUtils.isEnabled()) {
+        final String effectiveSolrConfigId = solrConfig.effectiveId(configSet.getName());
+        jerseyAppHandler =
+            coreContainer
+                .getAppHandlerCache()
+                .computeIfAbsent(
+                    effectiveSolrConfigId,
+                    () -> {
+                      log.debug(

Review Comment:
   add log.isDebugEnabled()`



##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -192,11 +193,16 @@ public CoreLoadFailure(CoreDescriptor cd, Exception loadFailure) {
       new PluginBag<>(SolrRequestHandler.class, null);
 
   private volatile ApplicationHandler jerseyAppHandler;
+  private volatile JerseyAppHandlerCache appHandlersByConfigSetId;
 
   public ApplicationHandler getJerseyApplicationHandler() {
     return jerseyAppHandler;
   }
 
+  public JerseyAppHandlerCache getAppHandlerCache() {

Review Comment:
   this name `getAppHandlerCache()` is not clear enough. please consider `getJerseyHandlers()` or something like that



##########
solr/core/src/java/org/apache/solr/core/ConfigOverlay.java:
##########
@@ -34,15 +34,15 @@
  * performed on tbhis gives a new copy of the object with the changed value
  */
 public class ConfigOverlay implements MapSerializable {
-  private final int znodeVersion;

Review Comment:
   is there any reason why this rename was necessary ?



##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
   public ConfigNode get(String name, Predicate<ConfigNode> test) {
     return root.get(name, test);
   }
+
+  /**
+   * Generates a String ID to represent the {@link ConfigSet}
+   *
+   * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+   * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+   * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+   * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+   * result.
+   *
+   * @param configSetName the name of the configSet that this SolrConfig belongs to.
+   */
+  public String effectiveId(String configSetName) {
+    return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+  }

Review Comment:
   > I'm skeptical configSetName even needs to be a part of this.
   
   I think it should be a part of the ID.
   
   > we don't see a use case of sharing SolrConfigs between configSets
   agree
   
   
   
   



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