SOLR-16615: Reuse Jersey apps for cores with the same configset #1286

gerlowskija · 2023-01-10T21:58:25Z

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:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
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

This commit introduces the first pieces needed to share Jersey applications across cores which share a configset. This is done by moving ownership of the Jersey 'ApplicationHandler' from the SolrCore object itself to a new 'JerseyAppHandlerCache' class. Reference counts are used to track how many SolrCores are currently relying on a particular ApplicationHandler; ApplicationHandlers are removed from the cache when this reference count hits 0. In order to support multiple sharing strategies, this commit also introduces the ConfigsetIdGenerator abstraction. Each Generator implementation takes in a 'ConfigSet' and a 'SolrCore' and produces a String ID that is used to fetch the Jersey application from the cache. Two implementations are provided: NoSharingConfigsetIdGenerator uses the SolrCore's UUID as its ID (ensuring that no app-sharing will occur), and ConfigsetNameIdGenerator uses the name of the configset as its ID (ensuring that all cores using that configset will share the same Jersey application). This initial pass has several limitations that need further investigation: - Relying on the configset name alone is insufficient, as configsets can be changed "in place" via ZK or filesystem changes. A more nuanced 'ConfigsetIdGenerator' impl will be needed here, perhaps that hashes the contents of the configset or something similarly content aware? - SolrCoreJerseyApp needs new injectors to make sure it can find the right SolrCore and metrics objects at request time.

The prior mechanism for injecting SolrCore instances into Jersey resources used a singleton factory, and took advantage of the fact that each ApplicationHandler was specific to a single core. This commit replaces the singleton-factory used for injection with a more standard factory that lifts the SolrCore out of the request context.

gerlowskija · 2023-01-10T22:01:05Z

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.

We want to share a Jersey appHandler among all cores that have the same config, but this requires a deeper check than just looking at the name of the configset. The "effective configset" used by a core really combines a number of different pieces of state: the configset files themselves (primarily solrconfig.xml), any core or collection properties that are used as variables in solrconfig.xml, and any config modifications made using the "overlay" API. This commit attempts to update ConfigsetIdGenerator to take each of these chunks of state into account when generating an ID for the core+configset combination.

gerlowskija · 2023-01-11T20:28:49Z

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.

risdenk · 2023-01-12T17:19:32Z

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!

HoustonPutman

This looks good to me!

I think a test showing that the cores are using the same application (like looking at the cache size), would really complete this PR.

solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java

Co-authored-by: Kevin Risden <risdenk@users.noreply.github.com>

gerlowskija · 2023-01-13T20:27:49Z

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.

HoustonPutman · 2023-01-13T20:51:38Z

solr/core/src/test/org/apache/solr/jersey/JerseyApplicationSharingTest.java

+  private void assertJerseyAppCacheHasSize(int expectedSize) {
+    assertEquals(
+        expectedSize,
+        cluster.getJettySolrRunners().get(0).getCoreContainer().getAppHandlerCache().size());


Can we get an assert message here?

HoustonPutman

Love the test. This looks good to me!

gerlowskija · 2023-01-18T00:30:59Z

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 precommit fix that you were nice enough to 'Suggest' and 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!

risdenk

All looks good to me - didn't mean to hold anything up.

Prior to this commit our Jersey integration involved 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. 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 SolrCores.

solr/core/src/java/org/apache/solr/core/ConfigOverlay.java

dsmiley · 2023-01-23T22:12:37Z

solr/core/src/java/org/apache/solr/core/ConfigOverlay.java

-  private final int znodeVersion;
+  private final int version;


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

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.

solr/core/src/java/org/apache/solr/core/SolrConfig.java

solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java

solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java

solr/core/src/java/org/apache/solr/core/SolrCore.java

dsmiley · 2023-01-23T22:52:27Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java


  public ApplicationHandler getJerseyApplicationHandler() {
    return jerseyAppHandler;
  }

+  public JerseyAppHandlerCache getAppHandlerCache() {


No Jersey in the name? I like the clarity of putting Jersey in the name.

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.

dsmiley · 2023-01-23T22:54:16Z

solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java

+              log.info(
+                  "Removing AppHandler from cache for 'effective configset' [{}]",
+                  effectiveConfigSetId);


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

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.

dsmiley · 2023-01-23T23:01:47Z

solr/core/src/java/org/apache/solr/jersey/JerseyAppHandlerCache.java

+              log.info(
+                  "Removing AppHandler from cache for 'effective configset' [{}]",
+                  effectiveConfigSetId);
+              applicationByConfigSetId.remove(effectiveConfigSetId);


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.

OK. Let me restate the race to make sure I understand:

coreA is created and puts its AppHandler in the cache (with refCount=1)

The Solr node receives a 'create' for coreB and a 'delete' for coreA come in in close proximity

coreB fetches the existing appHandler from the cache (but hasn't yet incref'd and returned)

coreA is deleted and calls its decref()

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.

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.

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.

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

gerlowskija · 2023-01-25T14:07:43Z

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 for review later this morning.

gerlowskija · 2023-01-25T17:33:22Z

Turns out there isn't any way in GH to reopen merged PRs, so I've opened a new PR (here) to continue iteration on this functionality. Planning to address David's comments (with some help from Houston) over there.

gerlowskija added 2 commits January 10, 2023 08:49

gerlowskija marked this pull request as draft January 10, 2023 21:58

HoustonPutman and others added 9 commits January 10, 2023 21:53

Add a hash for solrConfig

c7948f8

Remove singleton-factory for v2 metrics filtering

69fea3b

Run 'tidy'

6ef33c8

Rename JerseyMetricsLookupFactory and related fields

d979433

Nuke unused MetricBeanFactory

1712c08

Misc cleanup

7d224c4

Remove ConfigsetIdGenerator abstraction

5aa5968

Add javadocs to JerseyAppHandlerCache

81dfc58

gerlowskija marked this pull request as ready for review January 11, 2023 20:28

HoustonPutman reviewed Jan 12, 2023

View reviewed changes

risdenk requested changes Jan 12, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java Outdated Show resolved Hide resolved

gerlowskija and others added 3 commits January 13, 2023 10:45

Small precommit fix

730ac67

Co-authored-by: Kevin Risden <risdenk@users.noreply.github.com>

Merge branch 'main' into SOLR-16615-reuse-jersey-apps

ddb5a8d

Add itest for ApplicationHandler cache

74e5237

HoustonPutman reviewed Jan 13, 2023

View reviewed changes

HoustonPutman approved these changes Jan 13, 2023

View reviewed changes

gerlowskija requested a review from risdenk January 18, 2023 00:27

risdenk approved these changes Jan 18, 2023

View reviewed changes

gerlowskija merged commit c21719e into apache:main Jan 23, 2023

gerlowskija deleted the SOLR-16615-reuse-jersey-apps branch January 23, 2023 16:59

dsmiley reviewed Jan 23, 2023

View reviewed changes

gerlowskija mentioned this pull request Jan 25, 2023

SOLR-16615: Reinstate Jersey app-per-configset #1314

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16615: Reuse Jersey apps for cores with the same configset #1286

SOLR-16615: Reuse Jersey apps for cores with the same configset #1286

gerlowskija commented Jan 10, 2023 •

edited

gerlowskija commented Jan 10, 2023

gerlowskija commented Jan 11, 2023

risdenk commented Jan 12, 2023

HoustonPutman left a comment

gerlowskija commented Jan 13, 2023

HoustonPutman Jan 13, 2023

HoustonPutman left a comment

gerlowskija commented Jan 18, 2023 •

edited

risdenk left a comment

dsmiley Jan 23, 2023

HoustonPutman Jan 24, 2023

dsmiley Jan 23, 2023

gerlowskija Jan 25, 2023

dsmiley Jan 23, 2023

gerlowskija Jan 25, 2023

dsmiley Jan 23, 2023

gerlowskija Jan 25, 2023

gerlowskija Jan 25, 2023

gerlowskija Jan 26, 2023

gerlowskija commented Jan 25, 2023 •

edited

gerlowskija commented Jan 25, 2023

SOLR-16615: Reuse Jersey apps for cores with the same configset #1286

SOLR-16615: Reuse Jersey apps for cores with the same configset #1286

Conversation

gerlowskija commented Jan 10, 2023 • edited

Description

Solution

Tests

Checklist

gerlowskija commented Jan 10, 2023

gerlowskija commented Jan 11, 2023

risdenk commented Jan 12, 2023

HoustonPutman left a comment

Choose a reason for hiding this comment

gerlowskija commented Jan 13, 2023

Choose a reason for hiding this comment

HoustonPutman left a comment

Choose a reason for hiding this comment

gerlowskija commented Jan 18, 2023 • edited

risdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija commented Jan 25, 2023 • edited

gerlowskija commented Jan 25, 2023

gerlowskija commented Jan 10, 2023 •

edited

gerlowskija commented Jan 18, 2023 •

edited

gerlowskija commented Jan 25, 2023 •

edited