gus-asf commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1609932690
##########
solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java:
##########
@@ -93,69 +87,69 @@ public class CoreContainerProvider implements
ServletContextListener {
private HttpClient httpClient;
private SolrMetricManager metricManager;
private RateLimitManager rateLimitManager;
- private final CountDownLatch init = new CountDownLatch(1);
private String registryName;
- // AFAIK the only reason we need this is to support JettySolrRunner for
tests. In tests we might
- // have multiple CoreContainers in the same JVM, but I *think* that doesn't
happen in a real
- // server.
- private static final Map<ContextInitializationKey, ServiceHolder> services =
- Collections.synchronizedMap(new WeakHashMap<>());
-
- // todo: dependency injection instead, but for now this method and the
associated map will have
- // to suffice.
- // Note that this relies on ServletContext.equals() not implementing
anything significantly
- // different than Object.equals for its .equals method (I've found no
implementation that even
- // implements it).
- public static ServiceHolder serviceForContext(ServletContext ctx) throws
InterruptedException {
- ContextInitializationKey key = new ContextInitializationKey(ctx);
- return services.computeIfAbsent(key, ServiceHolder::new);
+
+ /** Acquires an instance from the context, waiting if necessary. */
+ public static CoreContainerProvider serviceForContext(ServletContext ctx)
+ throws InterruptedException {
+ long startWait = System.nanoTime();
+ synchronized (ctx) {
Review Comment:
Hmm interesting idea, though slightly scary to wait on something jetty owns.
Never know when they might also (try to) lock it during server initialization.
Also I'm unsure if this would be safe if we ever had another
ServletContextListner do the same thing... Worse yet the use of JettySolrRunner
for our tests means that we don't have any tests for what start.jar is doing.
That dichotomy is a lurking problem, for which there are 2 possible solutions:
1) make jetty startup quicker for tests with
https://eclipse.dev/jetty/documentation/jetty-12/operations-guide/index.html#og-quickstart
and ditch JettySolrRunner or 2) ditching start.jar and transitioning to custom
programatic jetty startup (possibly a variant of JettySolrRunner). As it
stands, the tests don't do a good job of covering the true startup process.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
