[GitHub] [solr] dsmiley commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset

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

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


##########
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;
+  private final int version;

Review Comment:
   "znodeVersion" is quite popular.  Are you trying to make this more generic?



##########
solr/core/src/java/org/apache/solr/core/ConfigOverlay.java:
##########
@@ -267,9 +267,14 @@ public ConfigOverlay deleteNamedPlugin(String name, String typ) {
     Map<?, ?> reqHandler = (Map<?, ?>) dataCopy.get(typ);
     if (reqHandler == null) return this;
     reqHandler.remove(name);
-    return new ConfigOverlay(dataCopy, this.znodeVersion);
+    return new ConfigOverlay(dataCopy, this.version);
   }
 
   public static final String ZNODEVER = "znodeVersion";
   public static final String NAME = "overlay";
+
+  @Override
+  public int hashCode() {
+    return data.hashCode();
+  }

Review Comment:
   Okay, but where is equals?



##########
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:
   No Jersey in the name?  I like the clarity of putting Jersey in the name.



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.util.RefCounted;
+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 {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private final Map<String, RefCounted<ApplicationHandler>> applicationByConfigSetId;
+
+  public JerseyAppHandlerCache() {
+    this.applicationByConfigSetId = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * Return the 'ApplicationHandler' associated with the provided ID, creating it first if
+   * necessary.
+   *
+   * <p>This method is thread-safe by virtue of its delegation to {@link
+   * ConcurrentHashMap#computeIfAbsent(Object, Function)} internally.
+   *
+   * @param effectiveConfigSetId an ID to associate the ApplicationHandler with. Usually created via
+   *     {@link #generateIdForConfigSet(ConfigSet)}.
+   * @param createApplicationHandler a Supplier producing an ApplicationHandler
+   */
+  public RefCounted<ApplicationHandler> computeIfAbsent(
+      String effectiveConfigSetId, Supplier<ApplicationHandler> createApplicationHandler) {
+    final Function<String, RefCounted<ApplicationHandler>> wrapper =
+        s -> {
+          return new RefCounted<>(createApplicationHandler.get()) {
+            @Override
+            public void close() {
+              log.info(
+                  "Removing AppHandler from cache for 'effective configset' [{}]",
+                  effectiveConfigSetId);
+              applicationByConfigSetId.remove(effectiveConfigSetId);

Review Comment:
   There is a race condition here if a core closes at the same time one is created with the same need of this handler.  The thread creating a core could fetch from the ConcurrentHashMap.computeIfAbsent but hasn't _quite_ called incref() yet.  Then the other thread calling `decref` will close this here and remove.  I'm not sure what the consequence is yet.



##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -358,11 +358,14 @@ public InputStream openResource(String resource) throws IOException {
       // The resource is either inside instance dir or we allow unsafe loading, so allow testing if
       // file exists
       if (Files.exists(inConfigDir) && Files.isReadable(inConfigDir)) {
-        return Files.newInputStream(inConfigDir);
+        return new SolrFileInputStream(
+            Files.newInputStream(inConfigDir), Files.getLastModifiedTime(inConfigDir).toMillis());

Review Comment:
   let's add a convenience constructor to SolrFileInputStream



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.util.RefCounted;
+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 {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private final Map<String, RefCounted<ApplicationHandler>> applicationByConfigSetId;
+
+  public JerseyAppHandlerCache() {
+    this.applicationByConfigSetId = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * Return the 'ApplicationHandler' associated with the provided ID, creating it first if
+   * necessary.

Review Comment:
   I think it's important for methods that return RefCounted things, like this, to declare that the caller must arrange for the object to be decref'ed when its done with it.



##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -178,9 +179,14 @@ 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());
         this.fileName = zkin.fileName;
       }
+      if (in instanceof SolrResourceLoader.SolrFileInputStream) {

Review Comment:
   I'd prefer an else-if style clarifying only one customization of the logic could applies.  But moreover, there is clearly a bigger issue beyond the scope of this PR to better abstract resource version information in a way that doesn't involve casting.  This is not the only spot I've seen it; I've seen it in versioning the schema and something else.  Perhaps create a VersionedInputStream interface providing both a long for creation and modification.



##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -984,4 +987,43 @@ public static void persistConfLocally(
   // This is to verify if this requires to use the schema classloader for classes loaded from
   // packages
   private static final ThreadLocal<ResourceLoaderAware> CURRENT_AWARE = new ThreadLocal<>();
+
+  public static class SolrFileInputStream extends InputStream {

Review Comment:
   Should extend FilterInputStream!  Then you needn't override so many things.



##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1178,9 @@ public ConfigNode get(String name) {
   public ConfigNode get(String name, Predicate<ConfigNode> test) {
     return root.get(name, test);
   }
+
+  @Override
+  public int hashCode() {
+    return hashCode;
+  }

Review Comment:
   again; without equals?  I suppose its legal but admittedly unusual.



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.util.RefCounted;
+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 {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private final Map<String, RefCounted<ApplicationHandler>> applicationByConfigSetId;
+
+  public JerseyAppHandlerCache() {
+    this.applicationByConfigSetId = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * Return the 'ApplicationHandler' associated with the provided ID, creating it first if
+   * necessary.
+   *
+   * <p>This method is thread-safe by virtue of its delegation to {@link
+   * ConcurrentHashMap#computeIfAbsent(Object, Function)} internally.
+   *
+   * @param effectiveConfigSetId an ID to associate the ApplicationHandler with. Usually created via
+   *     {@link #generateIdForConfigSet(ConfigSet)}.
+   * @param createApplicationHandler a Supplier producing an ApplicationHandler
+   */
+  public RefCounted<ApplicationHandler> computeIfAbsent(
+      String effectiveConfigSetId, Supplier<ApplicationHandler> createApplicationHandler) {
+    final Function<String, RefCounted<ApplicationHandler>> wrapper =
+        s -> {
+          return new RefCounted<>(createApplicationHandler.get()) {
+            @Override
+            public void close() {
+              log.info(
+                  "Removing AppHandler from cache for 'effective configset' [{}]",
+                  effectiveConfigSetId);
+              applicationByConfigSetId.remove(effectiveConfigSetId);
+            }
+          };
+        };
+
+    final RefCounted<ApplicationHandler> fetched =
+        applicationByConfigSetId.computeIfAbsent(effectiveConfigSetId, wrapper);
+    fetched.incref();
+    return fetched;
+  }
+
+  public int size() {
+    return applicationByConfigSetId.size();
+  }
+
+  /**
+   * Generates a String ID to represent the provided {@link ConfigSet}
+   *
+   * <p>Relies on {@link SolrConfig#hashCode()} to generate a different ID for each "unique"
+   * configset (where "uniqueness" considers various overlays that get applied to the {@link
+   * ConfigSet})
+   *
+   * @see SolrCore#hashCode()
+   */
+  public static String generateIdForConfigSet(ConfigSet configSet) {
+    return configSet.getName() + "-" + configSet.getSolrConfig().hashCode();
+  }

Review Comment:
   Seems to me this method belongs on ConfigSet



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.util.RefCounted;
+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 {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private final Map<String, RefCounted<ApplicationHandler>> applicationByConfigSetId;
+
+  public JerseyAppHandlerCache() {
+    this.applicationByConfigSetId = new ConcurrentHashMap<>();
+  }

Review Comment:
   nit: you could simply initialize at the declaration.



##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -225,7 +226,7 @@ public class SolrCore implements SolrInfoBean, Closeable {
   private final Date startTime = new Date();
   private final long startNanoTime = System.nanoTime();
   private final RequestHandlers reqHandlers;
-  private final ApplicationHandler jerseyAppHandler;
+  private final RefCounted<ApplicationHandler> appHandlerForConfigSet;

Review Comment:
   I like that it had "jersey" in its name before, as we've already spoken about.  Can you put it back please?
   Not sure if we need the "ForConfigSet" part in the name; a comment would be fine for that.  For example schemas can be shared as well -- and it's by configSet.



##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.util.RefCounted;
+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 {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private final Map<String, RefCounted<ApplicationHandler>> applicationByConfigSetId;
+
+  public JerseyAppHandlerCache() {
+    this.applicationByConfigSetId = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * Return the 'ApplicationHandler' associated with the provided ID, creating it first if
+   * necessary.
+   *
+   * <p>This method is thread-safe by virtue of its delegation to {@link
+   * ConcurrentHashMap#computeIfAbsent(Object, Function)} internally.
+   *
+   * @param effectiveConfigSetId an ID to associate the ApplicationHandler with. Usually created via
+   *     {@link #generateIdForConfigSet(ConfigSet)}.
+   * @param createApplicationHandler a Supplier producing an ApplicationHandler
+   */
+  public RefCounted<ApplicationHandler> computeIfAbsent(
+      String effectiveConfigSetId, Supplier<ApplicationHandler> createApplicationHandler) {
+    final Function<String, RefCounted<ApplicationHandler>> wrapper =
+        s -> {
+          return new RefCounted<>(createApplicationHandler.get()) {
+            @Override
+            public void close() {
+              log.info(
+                  "Removing AppHandler from cache for 'effective configset' [{}]",
+                  effectiveConfigSetId);

Review Comment:
   I'm sure this is interesting to you in development of this feature, but it'll be noise for the rest of us.



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