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