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

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

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


##########
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:
   This feels off.  "Generates a String ID to represent the ConfigSet".  No... this is on SolrConfig; it should be an ID to represent the ID of this SolrConfig.  I'm skeptical configSetName even needs to be a part of this.  Sure in practice it will will be by-configSet but as a by-product of configSets having different configs.  If there is a reason ApplicationHandler needs to be by ConfigSet even if there may be identical solrconfig's (which I could imagine being so) then I think it's the caller of this method to concatenate the configSet name.  Or build the configSet's identity into an existing field to reference so that it's not passed in (I less prefer).



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.jersey;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.lang.invoke.MethodHandles;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.glassfish.jersey.server.ApplicationHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores Jersey 'ApplicationHandler' instances by an ID or hash derived from their {@link
+ * ConfigSet}.
+ *
+ * <p>ApplicationHandler creation is expensive; caching these objects allows them to be shared by
+ * multiple cores with the same configuration.
+ */
+public class JerseyAppHandlerCache {

Review Comment:
   This class now seems useless -- and that's a good thing :-). Less to name & document -- can be done so more succinctly in-context.



##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -225,6 +225,7 @@ public class SolrCore implements SolrInfoBean, Closeable {
   private final Date startTime = new Date();
   private final long startNanoTime = System.nanoTime();
   private final RequestHandlers reqHandlers;
+  // RefCounted because multiple cores using the same configset may share an AppHandler

Review Comment:
   No; isn't.



##########
solr/core/src/java/org/apache/solr/jersey/InjectionFactories.java:
##########
@@ -62,6 +65,25 @@ public SolrQueryResponse provide() {
     public void dispose(SolrQueryResponse instance) {}
   }
 
+  /** Fetch the (existing) SolrCore from the request context */
+  public static class SolrCoreFactory implements Factory<SolrCore> {

Review Comment:
   FYI the name of this suggests something it most definitely isn't but I can be okay with it based on it's an inner class of something establishing context.



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