You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2024/05/22 04:15:28 UTC
[PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
dsmiley opened a new pull request, #2474:
URL: https://github.com/apache/solr/pull/2474
Thereby fixing some rare bugs in tests, maybe in the real world too.
https://issues.apache.org/jira/browse/SOLR-17118
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2474:
URL: https://github.com/apache/solr/pull/2474#issuecomment-2135435742
If I get no more feedback, I'll merge Friday.
I'm not sure what single test (or small number of such) to beast. Running all tests isn't viable.
Suggested CHANGES.txt in "Bugs":
* Simplify CoreContainerProvider initialization and JettySolrRunner. Avoid deadlock/hang.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2474:
URL: https://github.com/apache/solr/pull/2474#issuecomment-2141246716
And another hang in another of my PRs: https://github.com/apache/solr/pull/2487#issuecomment-2141245307
Looking forward to merging this today/Friday :-) and not waiting *that* long to bring to 9x.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1611685627
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -556,15 +528,7 @@ public synchronized void start(boolean reusePort) throws Exception {
server.start();
}
}
- synchronized (JettySolrRunner.this) {
Review Comment:
after server.start returns, dispatchFilter should be running. It's not started asynchronously; no race.
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -375,24 +370,11 @@ private void init(int port) {
new ServletContextHandler(server, "/solr", ServletContextHandler.SESSIONS);
root.setResourceBase(".");
- server.addEventListener(
- new LifeCycle.Listener() {
-
- @Override
- public void lifeCycleStopping(LifeCycle arg0) {}
-
- @Override
- public synchronized void lifeCycleStopped(LifeCycle arg0) {
- if (coreContainerProvider != null) {
- coreContainerProvider.close();
- }
- }
-
+ root.addEventListener(
+ new CoreContainerProvider() {
Review Comment:
Notice I'm subclassing CCP in order do some initialization
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -726,10 +674,6 @@ public synchronized void stop() throws Exception {
}
}
- private ExecutorService getJettyShutDownThreadPool() {
Review Comment:
an aside, I hate supposed getters that in fact yield new things always
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -402,41 +384,31 @@ public synchronized void lifeCycleStarted(LifeCycle arg0) {
.setAttribute(SolrDispatchFilter.PROPERTIES_ATTRIBUTE, nodeProperties);
root.getServletContext()
.setAttribute(SolrDispatchFilter.SOLRHOME_ATTRIBUTE, solrHome);
+
SSLConfigurationsFactory.current().init(); // normally happens in jetty-ssl.xml
- coreContainerProvider = new CoreContainerProvider();
- coreContainerProvider.init(root.getServletContext());
+
log.info("Jetty properties: {}", nodeProperties);
- debugFilter =
- root.addFilter(DebugFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
- extraFilters = new ArrayList<>();
- for (Map.Entry<Class<? extends Filter>, String> entry :
- config.extraFilters.entrySet()) {
- extraFilters.add(
- root.addFilter(
- entry.getKey(), entry.getValue(), EnumSet.of(DispatcherType.REQUEST)));
- }
-
- for (Map.Entry<ServletHolder, String> entry : config.extraServlets.entrySet()) {
- root.addServlet(entry.getKey(), entry.getValue());
- }
- dispatchFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- dispatchFilter.setHeldClass(SolrDispatchFilter.class);
- dispatchFilter.setInitParameter("excludePatterns", excludePatterns);
- // Map dispatchFilter in same path as in web.xml
- root.addFilter(dispatchFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
-
- synchronized (JettySolrRunner.this) {
- waitOnSolr = true;
- JettySolrRunner.this.notify();
- }
- }
-
- @Override
- public void lifeCycleFailure(LifeCycle arg0, Throwable arg1) {
- System.clearProperty("hostPort");
+ super.contextInitialized(event);
}
});
+
+ debugFilter = root.addFilter(DebugFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
Review Comment:
No longer is this section in a LifeCycle.Listener. No longer does it do JettySolrRunner.this.notify(); waitOnSolr is gone.
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -661,21 +625,11 @@ public synchronized void stop() throws Exception {
Map<String, String> prevContext = MDC.getCopyOfContextMap();
MDC.clear();
try {
- Filter filter = dispatchFilter.getFilter();
QueuedThreadPool qtp = (QueuedThreadPool) server.getThreadPool();
ReservedThreadExecutor rte = qtp.getBean(ReservedThreadExecutor.class);
server.stop();
- if (server.getState().equals(Server.FAILED)) {
Review Comment:
because we install these components before Jetty starts in a normal way now
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -699,12 +653,6 @@ public synchronized void stop() throws Exception {
timeout.waitFor("Timeout waiting for reserved executor to stop.", rte::isStopped);
}
- // we want to shutdown outside of jetty cutting us off
Review Comment:
as I said in the commit message for this change, this block here has been pointless ever since the previous big change by Gus. The comment dates back to @markrmiller but it's not clear to me way way back then what the concern was. Any way, I've found Jetty to do stuff fairly predictably and in one thread that I can reason about the sequencing.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1610754549
##########
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:
> Worse yet the use of JettySolrRunner for our tests means that we don't have any tests for what start.jar is doing.
BATS integration tests is our answer to that. JettySolrRunner just needs to handle the sequencing realistically (what starts and stops when). You might have also noticed some bolted-on requirements for tests like debug filter, proxy port, and more, so I don't see JettySolrRunner going away. It could be simplified further!
I'm not optimistic about that QuickStart thing helping us -- we've already tuned Jetty. Most notably, our web.xml has `metadata-complete="true"` thus there is no annotation scanning.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1610783362
##########
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:
Can you please recommend the beasting options I should use today/tonight?
I'd like to merge this to main only and then only backport after awhile.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1610784556
##########
solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java:
##########
@@ -112,36 +112,6 @@ public void testHealthCheckHandler() throws Exception {
newJetty.stop();
}
- // add a new node for the purpose of negative testing
- // negative check that if core container is not available at the node
- newJetty = cluster.startJettySolrRunner();
- try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
-
- // positive check that our (new) "healthy" node works with direct http client
- assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
-
- // shutdown the core container of new node
- newJetty.getCoreContainer().shutdown();
Review Comment:
I considered this portion of the test invalid since it quite intentionally (see the opening comment) shuts down the container without touching Jetty. That's simply not how Jetty shuts down; only Jetty's lifecycle initiates an orderly shutdown. I could test trying an attempt to do a request against a non-existent server but that seemed silly.
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -402,11 +385,20 @@ public synchronized void lifeCycleStarted(LifeCycle arg0) {
.setAttribute(SolrDispatchFilter.PROPERTIES_ATTRIBUTE, nodeProperties);
root.getServletContext()
.setAttribute(SolrDispatchFilter.SOLRHOME_ATTRIBUTE, solrHome);
+
SSLConfigurationsFactory.current().init(); // normally happens in jetty-ssl.xml
- coreContainerProvider = new CoreContainerProvider();
- coreContainerProvider.init(root.getServletContext());
+
log.info("Jetty properties: {}", nodeProperties);
+ super.contextInitialized(event);
+ }
+ });
+ // TODO the below could be done immediately; not be an event listener
Review Comment:
I'm holding myself back from scope creep :-). There's definitely more needless complexity I see
##########
solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java:
##########
@@ -112,36 +112,6 @@ public void testHealthCheckHandler() throws Exception {
newJetty.stop();
}
- // add a new node for the purpose of negative testing
- // negative check that if core container is not available at the node
- newJetty = cluster.startJettySolrRunner();
- try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
-
- // positive check that our (new) "healthy" node works with direct http client
- assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
-
- // shutdown the core container of new node
- newJetty.getCoreContainer().shutdown();
-
- // api shouldn't unreachable
- SolrException thrown =
- expectThrows(
- SolrException.class,
- () -> {
- req.process(solrClient).getResponse().get(CommonParams.STATUS);
- fail("API shouldn't be available, and fail at above request");
- });
- assertEquals("Exception code should be 404", 404, thrown.code());
- assertTrue(
- "Should have seen an exception containing the an error",
- thrown
- .getMessage()
- .contains(
- "Error processing the request. CoreContainer is either not initialized or shutting down."));
- } finally {
- newJetty.stop();
- }
-
Review Comment:
I guess I should remove the lower "redundant" part 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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1610765299
##########
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:
> Never know when they might also (try to) lock it during server initialization.
I'm not concerned; it's only Jetty and us to worry about. The code here is resilient to `wait` being notified inexplicably. There's a loop and it keeps checking for the CoreContainer to show up. Perhaps it'll log a bit more that it's waiting; it's okay. In practice I don't think it'll wait at all since you had pointed out in comments that the ServletContextListener is initialized first.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1611734387
##########
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:
Makes sense to me. If it doesn't work we can always change back right?
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1609909368
##########
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:
This was the sticky bit. Before pushing I suggest you beast the tests for a good while. My memory is that without this I got a low level of failures and or deadlocks. The current state while apparently not perfect had a low enough frequency that it didn't crop up. (though I don't remember exactly how long I was beasting things either).
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
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 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-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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #2474:
URL: https://github.com/apache/solr/pull/2474
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1611670535
##########
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:
Based on the initialization sequence that I'm coming to learn, I suspected that this code doesn't even have to wait and I was right! I added an assert before ctx.wait and it never tripped. Ha!
Maybe this is controversial but I'm tempted to remove this code (and the "notify" code elsewhere) and replace it with a lookup that on non-existence throws an IllegalStateException claiming that the ServletContextListener should have done its job by then. If it didn't, something is seriously wrong.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
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-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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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-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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1609936991
##########
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:
@epugh was the one who mentioned the jetty quickstart mechanism to me back when I was doing this. I don't know if he's had thoughts since then.
--
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
Re: [PR] SOLR-17118: Simplify CoreContainerProvider initialization [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2474:
URL: https://github.com/apache/solr/pull/2474#discussion_r1611732329
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -375,24 +370,11 @@ private void init(int port) {
new ServletContextHandler(server, "/solr", ServletContextHandler.SESSIONS);
root.setResourceBase(".");
- server.addEventListener(
- new LifeCycle.Listener() {
-
- @Override
- public void lifeCycleStopping(LifeCycle arg0) {}
-
- @Override
- public synchronized void lifeCycleStopped(LifeCycle arg0) {
- if (coreContainerProvider != null) {
- coreContainerProvider.close();
- }
- }
-
+ root.addEventListener(
+ new CoreContainerProvider() {
Review Comment:
maybe worth a comment in the code?
--
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