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