You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2023/01/10 21:58:27 UTC
[GitHub] [solr] gerlowskija opened a new pull request, #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
gerlowskija opened a new pull request, #1286:
URL: https://github.com/apache/solr/pull/1286
https://issues.apache.org/jira/browse/SOLR-16615
# Description
Currently, our Jersey integration involves creating an 'ApplicationHandler' class for each SolrCore. These classes are expensive and their creation can have a noticeable cumulative effect at startup time on nodes hosting many many cores. (See SOLR-16531 for more details.)
# Solution
This PR addresses this problem by allowing ApplicationHandler's to be shared across multiple SolrCores, as long as each of those cores use the same configset. To do this we introduce JerseyAppHandlerCache, which takes over ownership of all core-level ApplicationHandler's. AH's are ref-counted so that they can be removed from the cache and closed when no longer needed by any SolrCore's.
# Tests
TBD
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) 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.
- [x] 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.
- [ ] I have run `./gradlew check`.
- [ ] 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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1382353479
Alright, added the test you asked for @HoustonPutman and merged your precommit fix @risdenk . Thanks for the review everyone!
I'm going to post some perf data and instructions for reproducing over on SOLR-16531, and give folks a few days to reproduce if they'd like (a fun exercise for anyone interested in brushing up on `solr-bench`). But then I'll look to merge+backport 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
[GitHub] [solr] gerlowskija merged pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #1286:
URL: https://github.com/apache/solr/pull/1286
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1070031379
##########
solr/core/src/test/org/apache/solr/jersey/JerseyApplicationSharingTest.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.util.Map;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Tests ensuring that Jersey apps are shared between cores as expected.
+ *
+ * <p>Jersey applications should be shared by any cores on the same node that have the same
+ * "effective configset" (i.e. the same configset content and any overlays or relevant configuration
+ * properties)
+ */
+public class JerseyApplicationSharingTest extends SolrCloudTestCase {
+
+ private static final String collection = "collection1";
+ private static final String confDir = collection + "/conf";
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ configureCluster(1)
+ .addConfig("conf1", configset("cloud-minimal"))
+ .addConfig("conf2", configset("cloud-minimal"))
+ .configure();
+ }
+
+ @Test
+ public void testMultipleCoresWithSameConfigsetShareApplication() throws Exception {
+ final SolrClient solrClient = cluster.getSolrClient();
+
+ // No applications should be in the cache to start
+ assertJerseyAppCacheHasSize(0);
+
+ // All replicas for the created collection should share a single Jersey ApplicationHandler entry
+ // in the cache
+ final CollectionAdminRequest.Create coll1Create =
+ CollectionAdminRequest.createCollection("coll1", "conf1", 2, 2);
+ assertEquals(0, coll1Create.process(solrClient).getStatus());
+ assertJerseyAppCacheHasSize(1);
+
+ // A new collection using the same configset will also share the existing cached Jersey
+ // ApplicationHandler
+ final CollectionAdminRequest.Create coll2Create =
+ CollectionAdminRequest.createCollection("coll2", "conf1", 2, 2);
+ assertEquals(0, coll2Create.process(solrClient).getStatus());
+ assertJerseyAppCacheHasSize(1);
+
+ // Using a different configset WILL cause a new Jersey ApplicationHandler to be used (total
+ // cache-count = 2)
+ final CollectionAdminRequest.Create coll3Create =
+ CollectionAdminRequest.createCollection("coll3", "conf2", 2, 2);
+ assertEquals(0, coll3Create.process(solrClient).getStatus());
+ assertJerseyAppCacheHasSize(2);
+
+ // Modifying properties that affect a configset will also cause a new Jersey ApplicationHandler
+ // to be created (total cache-count = 3)
+ final CollectionAdminRequest.Create coll4Create =
+ CollectionAdminRequest.createCollection("coll4", "conf1", 2, 2);
+ coll4Create.setProperties(
+ Map.of(
+ "solr.commitwithin.softcommit",
+ "false")); // Set any collection property used in the cloud-minimal configset
+ assertEquals(0, coll4Create.process(solrClient).getStatus());
+ assertJerseyAppCacheHasSize(3);
+
+ // Deleting the only cores that used given ApplicationHandler will remove it from the cache
+ // (total cache-count = 2)
+ final CollectionAdminRequest.Delete deleteColl3 =
+ CollectionAdminRequest.deleteCollection("coll3");
+ assertEquals(0, deleteColl3.process(solrClient).getStatus());
+ assertJerseyAppCacheHasSize(2);
+ }
+
+ private void assertJerseyAppCacheHasSize(int expectedSize) {
+ assertEquals(
+ expectedSize,
+ cluster.getJettySolrRunners().get(0).getCoreContainer().getAppHandlerCache().size());
Review Comment:
Can we get an assert message here?
--
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
[GitHub] [solr] risdenk commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1380750054
Seeing this failure in github actions and locally:
```
1. ERROR in /home/runner/work/solr/solr/solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java (at line 45)
* PluginBag.JaxrsResourceToHandlerMappings}), and using that to look up the associated request
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Javadoc: Invalid member type qualification
```
overall like the way this looks!
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087096093
##########
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:
Done
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087128390
##########
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:
I'll need to think about this a bit more.
I think we could trivially fix this by making JerseyAppHandlerCache.computeIfAbsent a synchronized method, but that seems like a bit of a blunt instrument that would do a good bit of over-locking. Conversely we could create a different lock for each key (i.e. effective-configset ID) and do more fine-grained locking, but that feels complex for the risk and impact here.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1068404503
##########
solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java:
##########
@@ -40,9 +39,18 @@
/**
* A request and response filter used to initialize and report per-request metrics.
*
- * <p>Currently, JAX-RS v2 APIs rely on a {@link
- * org.apache.solr.handler.RequestHandlerBase.HandlerMetrics} instance from an associated request
- * handler.
+ * <p>Currently, Jersey resources that have a corresponding v1 API produce the same metrics as their
+ * v1 equivalent and rely on the v1 requestHandler instance to do so. Solr facilitates this by
+ * building a map of the JAX-RS resources to requestHandler mapping (a {@link
+ * PluginBag.JaxrsResourceToHandlerMappings}), and using that to look up the associated request
+ * handler (if one exists) in pre- and post- filters
Review Comment:
The suggestion includes the `tidy` reformatting too.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1069611652
##########
solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java:
##########
@@ -40,9 +39,18 @@
/**
* A request and response filter used to initialize and report per-request metrics.
*
- * <p>Currently, JAX-RS v2 APIs rely on a {@link
- * org.apache.solr.handler.RequestHandlerBase.HandlerMetrics} instance from an associated request
- * handler.
+ * <p>Currently, Jersey resources that have a corresponding v1 API produce the same metrics as their
+ * v1 equivalent and rely on the v1 requestHandler instance to do so. Solr facilitates this by
+ * building a map of the JAX-RS resources to requestHandler mapping (a {@link
+ * PluginBag.JaxrsResourceToHandlerMappings}), and using that to look up the associated request
+ * handler (if one exists) in pre- and post- filters
Review Comment:
Thanks Kevin! LGTM.
(This "Suggested change" feature in Github is pretty cool; I always forget it's there.)
--
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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1403678908
I merged this change on Monday but quickly reverted it when it caused some build failures (a bad bug crept in when resolving some merge conflicts).
I've since fixed almost all of those, and intend to put this back on 'main'. Collaboration-wise, I'm not sure the best way to do that. AFAICT I can't reopen this PR, since Github considers it "merged", but if I create a new PR I'll lose the review comments that @dsmiley put on this yesterday. Might have to bite the bullet on that if there's no trick to doing this, but I'll poke around a bit before pushing code up later this morning.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087098331
##########
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:
Done
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087123329
##########
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:
OK. Let me restate the race to make sure I understand:
1. coreA is created and puts its AppHandler in the cache (with refCount=1)
2. The Solr node receives a 'create' for coreB and a 'delete' for coreA come in in close proximity
3. coreB fetches the existing appHandler from the cache (but hasn't yet incref'd and returned)
4. coreA is deleted and calls its `decref()`
5. CoreA's call to decref() causes the AppHandler to be removed from the cache, even though coreB is about to get a reference to it.
6. coreB makes its incref() call and returns from JAHC.computeIfAbsent
It's definitely a race, good catch.
The most obvious impact here is that some other coreC created later would need to instantiate an AppHandler when it should've been able to reuse the one that coreB is already using.
That's the only impact that jumps out at me. I was concerned at first that if coreB was deleted later on, its decref call would end up causing coreC's AppHandler to be removed from the cache. But since ApplicationHandler doesn't implement `equals()` the cache-removal triggered when refCount==0 wouldn't actually do anything.
--
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
[GitHub] [solr] dsmiley commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1379446305
Alright - I've done much of the gap-filling and cleanup that I had in mind. Taking this out of "Draft" as it's truly ready for review at this point.
The last remaining bit here is testing. I already have some some unit tests written for JerseyAppHandlerCache, but they require a slight change to our mockito dep that I'd rather do in a separate PR (though we could def bundle it here too if desired). Hoping to add other tests as well.
But otherwise this should be all ready to go.
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1086857155
##########
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:
Yeah we should probably separate this into a different JIRA for better abstract resource version info.
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1085627272
##########
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:
Yep, because the version might not be a znode version, it might be an ending to the lastModified date from the filesystem (using a filesystem configset). There are other znodeVersion names I could have changed, but that would have been a lot more far-ranging than just changing this one for now.
In general for things that don't have to be a ZKConfigSet we should move from znodeVersion to version where feasible.
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1085629013
##########
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:
Perfect, I was looking for something like that but I guess I was searching for the wrong names.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1068403694
##########
solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java:
##########
@@ -40,9 +39,18 @@
/**
* A request and response filter used to initialize and report per-request metrics.
*
- * <p>Currently, JAX-RS v2 APIs rely on a {@link
- * org.apache.solr.handler.RequestHandlerBase.HandlerMetrics} instance from an associated request
- * handler.
+ * <p>Currently, Jersey resources that have a corresponding v1 API produce the same metrics as their
+ * v1 equivalent and rely on the v1 requestHandler instance to do so. Solr facilitates this by
+ * building a map of the JAX-RS resources to requestHandler mapping (a {@link
+ * PluginBag.JaxrsResourceToHandlerMappings}), and using that to look up the associated request
+ * handler (if one exists) in pre- and post- filters
Review Comment:
```suggestion
* org.apache.solr.core.PluginBag.JaxrsResourceToHandlerMappings}), and using that to look up the
* associated request handler (if one exists) in pre- and post- filters
```
apparently just need to be fully qualified.
--
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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1386285201
Hey @risdenk : I was hoping to gear up to merge, but see you still technically have an outstanding "change requested" on this PR. 99% sure this was the 'Suggested Change' to fix precommit that I accepted as-is, but I didn't want to assume in case you had something else you'd intended to mention?
Anything else you want to see here? If not I'll merge in a few days!
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1085625134
##########
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:
Yeah, good call. That part is on me.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087094844
##########
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:
I thought about putting it on 'ConfigSet' directly but ultimately opted to keep it here so all the logic remained together in one place.
I was also a little leery of making a "broader claim" than necessary here. This ID generation logic works perfect for our local needs in determining an "effective configset", but IMO it's totally plausible that other places in Solr might want to come up with configset IDs from some other criteria. I didn't want to elevate this approach by putting it 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<>();
+ }
+
+ /**
+ * Return the 'ApplicationHandler' associated with the provided ID, creating it first if
+ * necessary.
Review Comment:
Done
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087105926
##########
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 think this is at least potentially useful in case of some weird bug or interaction caused by the AppHandler reuse, but I'll definitely drop it down to be debug-level so no one else has to see the noise until they want it.
--
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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1377949310
This still lacks some needed polish - hoping to push some improvements for configset resolution and metrics-lookup later this evening. And of course tests and cleanup remain after that. But the skeleton of this functionality is all there and ready for review.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1088086507
##########
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:
To close the loop here - this race condition ended up going away in the new PR by removing RefCounting altogether (and using a Caffeine cache implementation, as suggested by David there).
--
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
[GitHub] [solr] gerlowskija commented on pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1286:
URL: https://github.com/apache/solr/pull/1286#issuecomment-1403986120
Turns out there isn't any way in GH to reopen merged PRs, so I've opened a new PR ([here](https://github.com/apache/solr/pull/1314)) to continue iteration on this functionality. Planning to address David's comments (with some help from Houston) over there.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087101643
##########
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:
I kept 'Jersey' from the getter name because we might swap Jersey out for some other JAX-RS implementation down the road, and the less we reference the name of the specific implementation the better.
I've been somewhat inconsistent on this though, so if you think it helps make things clearer then I'm happy to change the getter name.
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1286: SOLR-16615: Reuse Jersey apps for cores with the same configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1286:
URL: https://github.com/apache/solr/pull/1286#discussion_r1087098331
##########
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 removed 'jersey' from the name out of an awareness that we might swap Jersey out for some other JAX-RS implementation down the road, and the less we reference the name of the specific implementation the better.
But reading this again now I think I'm ready to flip-flop back to 'jerseyAppHandler'. Idk what I was thinking with the 'ForConfigSet' bit. Done
--
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