You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2023/01/25 17:30:15 UTC
[GitHub] [solr] gerlowskija opened a new pull request, #1314: SOLR-16615: Reinstate Jersey app-per-configset
gerlowskija opened a new pull request, #1314:
URL: https://github.com/apache/solr/pull/1314
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.
A [previous attempt at this functionality](https://github.com/apache/solr/pull/1286) was committed but then reverted due to test failures. This new PR reinstates the functionality after fixing a few bugs and addressing some additional review feedback.
# Tests
See diff for new JUnit tests.
# 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`.
- [x] I have added tests for my changes.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1089108454
##########
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 added Jersey to the name - that was something David suggested on the previous PR.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1089074431
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
> I'm skeptical configSetName even needs to be a part of this
I didn't have a single specific reason in mind. It just seemed prudent from an "abundance of caution" perspective. Maybe some bug will crop up where we'd want to eliminate AH sharing, maybe a paranoid admin running a multi-tenant service would want to ensure sharing doesn't happen, etc.
That said I don't feel strongly about _where_ the configset name gets added. I'll move it closer to the actual caching as suggested.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1404005863
(Credit to Kevin for pointing this out). Anyone familiar with the previous PR see what has changed since then with [this link](https://github.com/gerlowskija/solr/compare/c21719ee2245bceebe124e3d39def9deabd2430c...SOLR-16615-reinstate-app-per-configset2)
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088529821
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -178,8 +179,12 @@ private class ResourceProvider implements Function<String, InputStream> {
ZkSolrResourceLoader.ZkByteArrayInputStream zkin =
(ZkSolrResourceLoader.ZkByteArrayInputStream) in;
zkVersion = zkin.getStat().getVersion();
- hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getZnodeVersion());
+ hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getVersion());
Review Comment:
No idea, but that was the behavior before the PR. I'm unsure whether the hash computed here is even used.
##########
solr/core/src/java/org/apache/solr/core/ConfigOverlay.java:
##########
@@ -34,15 +34,15 @@
* performed on tbhis gives a new copy of the object with the changed value
*/
public class ConfigOverlay implements MapSerializable {
- private final int znodeVersion;
Review Comment:
Because its no longer necessarily the znode version, it could be the file "version" for a file-system based configSet.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1407051269
I'm kindof on the fence about a CHANGES.txt entry. Why would a user ever care about this? What would they do with the information? (i.e. Yes this fixes a perf regression, but it's one that was never released.)
I'll draft up an entry just mentioning that the Jersey AppHandlers are now shared where possible - I'll push that up shortly and then merge this so that Jenkins can test away over the weekend. If folks have thoughts about the CHANGES.txt entry or additional review, happy to address that all.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088308535
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
This feels off. "Generates a String ID to represent the ConfigSet". No... this is on SolrConfig; it should be an ID to represent the ID of this SolrConfig. I'm skeptical configSetName even needs to be a part of this. Sure in practice it will will be by-configSet but as a by-product of configSets having different configs. If there is a reason ApplicationHandler needs to be by ConfigSet even if there may be identical solrconfig's (which I could imagine being so) then I think it's the caller of this method to concatenate the configSet name. Or build the configSet's identity into an existing field to reference so that it's not passed in (I less prefer).
##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.jersey;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.lang.invoke.MethodHandles;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.glassfish.jersey.server.ApplicationHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores Jersey 'ApplicationHandler' instances by an ID or hash derived from their {@link
+ * ConfigSet}.
+ *
+ * <p>ApplicationHandler creation is expensive; caching these objects allows them to be shared by
+ * multiple cores with the same configuration.
+ */
+public class JerseyAppHandlerCache {
Review Comment:
This class now seems useless -- and that's a good thing :-). Less to name & document -- can be done so more succinctly in-context.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -225,6 +225,7 @@ public class SolrCore implements SolrInfoBean, Closeable {
private final Date startTime = new Date();
private final long startNanoTime = System.nanoTime();
private final RequestHandlers reqHandlers;
+ // RefCounted because multiple cores using the same configset may share an AppHandler
Review Comment:
No; isn't.
##########
solr/core/src/java/org/apache/solr/jersey/InjectionFactories.java:
##########
@@ -62,6 +65,25 @@ public SolrQueryResponse provide() {
public void dispose(SolrQueryResponse instance) {}
}
+ /** Fetch the (existing) SolrCore from the request context */
+ public static class SolrCoreFactory implements Factory<SolrCore> {
Review Comment:
FYI the name of this suggests something it most definitely isn't but I can be okay with it based on it's an inner class of something establishing context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] noblepaul commented on a diff in pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "noblepaul (via GitHub)" <gi...@apache.org>.
noblepaul commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088500824
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -178,8 +179,12 @@ private class ResourceProvider implements Function<String, InputStream> {
ZkSolrResourceLoader.ZkByteArrayInputStream zkin =
(ZkSolrResourceLoader.ZkByteArrayInputStream) in;
zkVersion = zkin.getStat().getVersion();
- hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getZnodeVersion());
+ hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getVersion());
Review Comment:
why is `zkin.getStat().getCtime()` used to compute hash? isn't `zkVersion` not enough?
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1136,10 +1137,23 @@ private SolrCore(
updateProcessorChains = loadUpdateProcessorChains();
reqHandlers = new RequestHandlers(this);
reqHandlers.initHandlersFromConfig(solrConfig);
- jerseyAppHandler =
- (V2ApiUtils.isEnabled())
- ? new ApplicationHandler(reqHandlers.getRequestHandlers().getJerseyEndpoints())
- : null;
+ if (V2ApiUtils.isEnabled()) {
+ final String effectiveSolrConfigId = solrConfig.effectiveId(configSet.getName());
+ jerseyAppHandler =
+ coreContainer
+ .getAppHandlerCache()
+ .computeIfAbsent(
+ effectiveSolrConfigId,
+ () -> {
+ log.debug(
Review Comment:
add log.isDebugEnabled()`
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -192,11 +193,16 @@ public CoreLoadFailure(CoreDescriptor cd, Exception loadFailure) {
new PluginBag<>(SolrRequestHandler.class, null);
private volatile ApplicationHandler jerseyAppHandler;
+ private volatile JerseyAppHandlerCache appHandlersByConfigSetId;
public ApplicationHandler getJerseyApplicationHandler() {
return jerseyAppHandler;
}
+ public JerseyAppHandlerCache getAppHandlerCache() {
Review Comment:
this name `getAppHandlerCache()` is not clear enough. please consider `getJerseyHandlers()` or something like that
##########
solr/core/src/java/org/apache/solr/core/ConfigOverlay.java:
##########
@@ -34,15 +34,15 @@
* performed on tbhis gives a new copy of the object with the changed value
*/
public class ConfigOverlay implements MapSerializable {
- private final int znodeVersion;
Review Comment:
is there any reason why this rename was necessary ?
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
> I'm skeptical configSetName even needs to be a part of this.
I think it should be a part of the ID.
> we don't see a use case of sharing SolrConfigs between configSets
agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1089074431
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
> In general if we don't see a use case of sharing SolrConfigs between configSets, then I don't really see a need to make that a possibility. @gerlowskija can weigh in, but I don't really want to open that rats nest.
I didn't have a single specific reason in mind. It just seemed prudent from an "abundance of caution" perspective. Maybe some bug will crop up where we'd want to eliminate AH sharing, maybe a paranoid admin running a multi-tenant service would want to ensure sharing doesn't happen, etc.
That said I don't feel strongly about _where_ the configset name gets added. I'll move it closer to the actual caching as suggested.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1405621031
Thanks for the remaining fixes Houston- LGTM. Running check locally now - would love to get this merged before the weekend!
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1405170234
Alright - perfect. This ended up being much much nicer I think!
I ended up using `Caffeine.newBuilder` directly instead of Solr's `CaffeineCache` wrapper. (`CaffeineCache` doesn't expose Caffeine's native support for weak-references afaict, and comes with a good bit of other baggage that we don't need here.). But it allows us to get rid of the ref-counting and all that by using "[weak reference eviction](https://github.com/ben-manes/caffeine/wiki/Eviction#reference-based)"
In eliminating ref-counting, it also clears up the race condition that David mentioned [here](https://github.com/apache/solr/pull/1286#discussion_r1084657648), so a win-win all around.
--
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 pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1406805287
Wait this definitely deserves a CHANGES.txt entry if it doesn't already have one
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088335316
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
Yeah the documentation was meant to say SolrConfig, not ConfigSet.
> I'm skeptical configSetName even needs to be a part of this.
I'm also skeptical, but saw no harm in adding it.
> then I think it's the caller of this method to concatenate the configSet name.
I did this originally then added the configSetName here. In general if we don't see a use case of sharing SolrConfigs between configSets, then I don't really see a need to make that a possibility. @gerlowskija can weigh in, but I don't really want to open that rats nest.
--
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] madrob commented on a diff in pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "madrob (via GitHub)" <gi...@apache.org>.
madrob commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088538119
##########
solr/core/src/java/org/apache/solr/core/ConfigOverlay.java:
##########
@@ -34,15 +34,15 @@
* performed on tbhis gives a new copy of the object with the changed value
*/
public class ConfigOverlay implements MapSerializable {
- private final int znodeVersion;
Review Comment:
If a znode with version 0 is deleted and recreated quickly then it comes back with the same version and this missed updates before
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088560118
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the {@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data (including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" + rootDataHashCode;
+ }
Review Comment:
I'm good with it being as a part of the ID so long as configSet is a field in the SolrConfig so that it's more clearly a part of its identity. Adding as a parameter on this method is suspicious.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1136,10 +1137,23 @@ private SolrCore(
updateProcessorChains = loadUpdateProcessorChains();
reqHandlers = new RequestHandlers(this);
reqHandlers.initHandlersFromConfig(solrConfig);
- jerseyAppHandler =
- (V2ApiUtils.isEnabled())
- ? new ApplicationHandler(reqHandlers.getRequestHandlers().getJerseyEndpoints())
- : null;
+ if (V2ApiUtils.isEnabled()) {
+ final String effectiveSolrConfigId = solrConfig.effectiveId(configSet.getName());
+ jerseyAppHandler =
+ coreContainer
+ .getAppHandlerCache()
+ .computeIfAbsent(
+ effectiveSolrConfigId,
+ () -> {
+ log.debug(
Review Comment:
Why? Note we have checks that can tell when it's helpful and when it isn't. It isn't helpful in this circumstance because the interpolated arguments are plain references -- not method calls that might do expensive stuff. The log.isDebugEnabled is verbose IMO for too little gain.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -178,8 +179,12 @@ private class ResourceProvider implements Function<String, InputStream> {
ZkSolrResourceLoader.ZkByteArrayInputStream zkin =
(ZkSolrResourceLoader.ZkByteArrayInputStream) in;
zkVersion = zkin.getStat().getVersion();
- hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getZnodeVersion());
+ hash = Objects.hash(zkin.getStat().getCtime(), zkVersion, overlay.getVersion());
Review Comment:
Note that a re-created file (removed then added) will have its zkVersion reset to 0. So incorporating CTime guards against 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] dsmiley commented on a diff in pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1089261762
##########
solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.jersey;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.lang.invoke.MethodHandles;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.solr.core.ConfigSet;
+import org.apache.solr.core.SolrConfig;
+import org.glassfish.jersey.server.ApplicationHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores Jersey 'ApplicationHandler' instances by an ID or hash derived from their {@link
+ * ConfigSet}.
+ *
+ * <p>ApplicationHandler creation is expensive; caching these objects allows them to be shared by
+ * multiple cores with the same configuration.
+ */
+public class JerseyAppHandlerCache {
Review Comment:
@gerlowskija WDYT?
--
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 pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1404214127
I don't think the ApplicationHandler needs to be RefCount'ed because there is no intrinsic closing work to do (unlike SolrIndexSearcher & SolrCore, say). RefCount'ing is no fun -- it's awkward; not an API I reach for unless there's no better option. We just don't want to retain them in memory; that's all. There are already utilities for this -- CaffeineCache with weak references.
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1089096003
##########
solr/core/src/java/org/apache/solr/jersey/InjectionFactories.java:
##########
@@ -62,6 +65,25 @@ public SolrQueryResponse provide() {
public void dispose(SolrQueryResponse instance) {}
}
+ /** Fetch the (existing) SolrCore from the request context */
+ public static class SolrCoreFactory implements Factory<SolrCore> {
Review Comment:
I'd like to keep "Factory" in there because it's implementing a Factory interface, but I agree the overloading of that term is unfortunate.
I can tweak the name so that it's clearer that this doesn't actually construct new SolrCore's though. That should help I think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #1314:
URL: https://github.com/apache/solr/pull/1314
--
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 #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1404255891
> I don't think the ApplicationHandler needs to be RefCount'ed because there is no intrinsic closing work to do (unlike SolrIndexSearcher & SolrCore, say). RefCount'ing is no fun -- it's awkward; not an API I reach for unless there's no better option. We just don't want to retain them in memory; that's all. There are already utilities for this -- CaffeineCache with weak references.
I'm not super familiar with CaffeineCache, but I'll look into it. Thanks for the suggestion @dsmiley
Two initial questions I'd have:
1. Does it bring along any sort of overhead? I see it's tracking a bunch of metrics and historical data for cache entries, computing RAM usage estimates, etc. Does that stuff have any non-negligible cost? Seemed reasonable to ask as performance is the driving motivation for this PR...
2. I understand the drive of switching from RefCounted to `WeakReference`. But doesn't that get us what we need right there? What does switching the enclosing data structure from ConcurrentHashMap to CaffeineCache get us - we don't need (or want?) cache-eviction or any of the other stuff that CC would bring, unless I'm missing some reason we'd need to evict? Feel like I'm missing some aspect of what you're suggesting...
--
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 pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1404309511
1. I believe its overhead is minimal; we use it already in Solr for many of our caches on the query path. We layer on our own metrics :-)
2. CHM doesn't have weak values natively -- Caffeine supports this option. It's an internal detail, not something we wrap ourselves, so is similar to understand/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 pull request #1314: SOLR-16615: Reinstate Jersey app-per-configset
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1314:
URL: https://github.com/apache/solr/pull/1314#issuecomment-1406691258
Alright, I think I addressed everyone's comments? Lmk what you think when you guys get a chance...
--
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