dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1609237802
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -167,8 +158,10 @@ public void init(FilterConfig config) throws
ServletException {
}
}
+ /** The CoreContainer. It's ready for use, albeit could shut down whenever.
Never null. */
+ // TODO update callers to skip pointless null checks
Review Comment:
tempting to address this TODO in the PR; I may.
##########
solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java:
##########
@@ -93,69 +87,71 @@ 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)
Review Comment:
We wait for it; previously did not. This allows us to return a bona-fide
CoreContainerProvider instead of a ServiceHolder wrapper. Alas, even CCP is
itself a wrapper to what we really want.
##########
solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java:
##########
@@ -93,69 +87,71 @@ 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) {
+ CoreContainerProvider provider;
+ while ((provider =
+ (CoreContainerProvider)
ctx.getAttribute(CoreContainerProvider.class.getName()))
+ == null) {
+ ctx.wait(1_000);
+ final int seconds = (int) ((System.nanoTime() - startWait) /
1_000_000_000);
+ if (seconds >= 2 && log.isInfoEnabled()) {
+ log.info("Still waiting for CoreContainer startup ({} seconds
elapsed)", seconds);
+ }
+ }
+ return provider;
+ }
}
@Override
- public void contextInitialized(ServletContextEvent sce) {
- init(sce.getServletContext());
+ public void contextInitialized(ServletContextEvent event) {
+ final var ctx = event.getServletContext();
+ init(ctx);
+ ctx.setAttribute(CoreContainerProvider.class.getName(), this);
+ // let serviceForContext() know
+ synchronized (ctx) {
+ ctx.notifyAll();
+ }
Review Comment:
This trick enables anyone waiting to know immediately that it's available.
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -73,10 +72,7 @@
public class SolrDispatchFilter extends BaseSolrFilter implements PathExcluder
{
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- // TODO: see if we can get rid of the holder here (Servlet spec actually
guarantees
- // ContextListeners run before filter init, but JettySolrRunner that we use
for tests is
- // complicated)
- private ServiceHolder coreService;
Review Comment:
good riddance
##########
solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java:
##########
@@ -93,69 +87,71 @@ 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) {
+ CoreContainerProvider provider;
+ while ((provider =
+ (CoreContainerProvider)
ctx.getAttribute(CoreContainerProvider.class.getName()))
+ == null) {
+ ctx.wait(1_000);
+ final int seconds = (int) ((System.nanoTime() - startWait) /
1_000_000_000);
+ if (seconds >= 2 && log.isInfoEnabled()) {
+ log.info("Still waiting for CoreContainer startup ({} seconds
elapsed)", seconds);
+ }
+ }
+ return provider;
+ }
}
@Override
- public void contextInitialized(ServletContextEvent sce) {
- init(sce.getServletContext());
+ public void contextInitialized(ServletContextEvent event) {
+ final var ctx = event.getServletContext();
+ init(ctx);
+ ctx.setAttribute(CoreContainerProvider.class.getName(), this);
+ // let serviceForContext() know
+ synchronized (ctx) {
+ ctx.notifyAll();
+ }
}
@Override
public void contextDestroyed(ServletContextEvent sce) {
close();
}
+ /**
+ * @see SolrDispatchFilter#getCores()
+ */
CoreContainer getCoreContainer() throws UnavailableException {
- waitForCoreContainer(() -> cores, init);
Review Comment:
Don't wait at this point anymore; it ought to be ready (so we check to
report a nice exception if it isn't)
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -378,21 +377,9 @@ private void init(int port) {
server.addEventListener(
new LifeCycle.Listener() {
- @Override
- public void lifeCycleStopping(LifeCycle arg0) {}
-
- @Override
- public synchronized void lifeCycleStopped(LifeCycle arg0) {
- if (coreContainerProvider != null) {
- coreContainerProvider.close();
- }
- }
-
- @Override
- public void lifeCycleStarting(LifeCycle arg0) {}
-
@Override
public synchronized void lifeCycleStarted(LifeCycle arg0) {
+ // awkwardly, parts of Solr want to know the port but we don't
know that until now
Review Comment:
probably something we could fix. The Jetty server inserts itself into the
context, thus CoreContainerProvider should be able to pull it out on
initialization in order to find the port.
--
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
