markrmiller commented on a change in pull request #155:
URL: https://github.com/apache/solr/pull/155#discussion_r650569434
##########
File path:
solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java
##########
@@ -611,27 +608,11 @@ public void stop() throws Exception {
Map<String,String> prevContext = MDC.getCopyOfContextMap();
MDC.clear();
try {
- Filter filter = dispatchFilter.getFilter();
-
// we want to shutdown outside of jetty cutting us off
- SolrDispatchFilter sdf = getSolrDispatchFilter();
- ExecutorService customThreadPool = null;
- if (sdf != null) {
- customThreadPool = ExecutorUtil.newMDCAwareCachedThreadPool(new
SolrNamedThreadFactory("jettyShutDown"));
-
- sdf.closeOnDestroy(false);
-// customThreadPool.submit(() -> {
-// try {
-// sdf.close();
-// } catch (Throwable t) {
-// log.error("Error shutting down Solr", t);
-// }
-// });
- try {
- sdf.close();
- } catch (Throwable t) {
- log.error("Error shutting down Solr", t);
- }
+ try {
Review comment:
This change is fine in general, but it has test and production shutdown
ramifications that I can't reason about mentally.
For tests, if the JettySolrRunner stop timeout is set to 0, it may not
change much, if I recall correctly, Jetty may not interrupt its executor
threads unless it's greater than 0. This was originally added to avoid those
interrupts. when the stop timeout is > 0. I wouldn't recommend that behavior,
but alterations do have a wide range of test and production stop affects.
If the production jetty configuration is still set to a 5 second stop
timeout, that means jetty will start interrupting its threads at 2.5 seconds,
and. with this change that. means interrupting Solr shutdown, which would now
be on a jetty pool thread. This was likely done because Solr behaved poorly in
tests in this case. So current ramifications unknown. Ideal case is that Solr
is fine and graceful in production and tests being interrupted on shutdown
though.
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -398,32 +397,12 @@ public CoreContainer getCores() {
@Override
public void destroy() {
- if (closeOnDestroy) {
- close();
- }
- }
-
- public void close() {
- CoreContainer cc = cores;
- cores = null;
try {
- if (metricManager != null) {
- try {
- metricManager.unregisterGauges(registryName, metricTag);
- } catch (NullPointerException e) {
- // okay
- } catch (Exception e) {
- log.warn("Exception closing FileCleaningTracker", e);
- } finally {
- metricManager = null;
- }
- }
- } finally {
- if (cc != null) {
- httpClient = null;
Review comment:
Nulling things can matter in rare circumstances in a perfect world. If
you don't have a thread linger of 10+ seconds set for tests, or don't want
unnecessary between test waits, the randomized testing framework will treat a
TERMINATED thread as a leaked thread and wait for it or fail on it and that
generally takes GC activity to clear out. Whether this is important here, don't
know. Don't think it matters with the current test config and setup anyway
though.
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -398,32 +397,12 @@ public CoreContainer getCores() {
@Override
public void destroy() {
- if (closeOnDestroy) {
- close();
- }
- }
-
- public void close() {
- CoreContainer cc = cores;
- cores = null;
try {
- if (metricManager != null) {
- try {
- metricManager.unregisterGauges(registryName, metricTag);
- } catch (NullPointerException e) {
- // okay
- } catch (Exception e) {
- log.warn("Exception closing FileCleaningTracker", e);
- } finally {
- metricManager = null;
- }
- }
- } finally {
- if (cc != null) {
- httpClient = null;
- cc.shutdown();
- }
+ cores.getMetricManager().unregisterGauges(registryName, metricTag);
Review comment:
I wonder about this - perhaps useful in tests, but pretty slow and
seemingly almost always unnecessary in real life. But unrelated.
##########
File path:
solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java
##########
@@ -611,27 +608,11 @@ public void stop() throws Exception {
Map<String,String> prevContext = MDC.getCopyOfContextMap();
MDC.clear();
try {
- Filter filter = dispatchFilter.getFilter();
-
// we want to shutdown outside of jetty cutting us off
- SolrDispatchFilter sdf = getSolrDispatchFilter();
- ExecutorService customThreadPool = null;
- if (sdf != null) {
- customThreadPool = ExecutorUtil.newMDCAwareCachedThreadPool(new
SolrNamedThreadFactory("jettyShutDown"));
-
- sdf.closeOnDestroy(false);
-// customThreadPool.submit(() -> {
-// try {
-// sdf.close();
-// } catch (Throwable t) {
-// log.error("Error shutting down Solr", t);
-// }
-// });
- try {
- sdf.close();
- } catch (Throwable t) {
- log.error("Error shutting down Solr", t);
- }
+ try {
+ dispatchFilter.stop();
Review comment:
Again, working around various test issues, ramifications today unknown.
Stop behavior is different than simply closing our stuff, theoretically fine,
but I don't know the random test behavior consequences in current code.
##########
File path:
solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java
##########
@@ -640,10 +621,9 @@ public void stop() throws Exception {
server.stop();
if (server.getState().equals(Server.FAILED)) {
- filter.destroy();
if (extraFilters != null) {
for (FilterHolder f : extraFilters) {
Review comment:
Sim to above, but likely fine.
##########
File path:
solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java
##########
@@ -436,7 +433,7 @@ protected HandlerWrapper injectJettyHandlers(HandlerWrapper
chain) {
* @return the {@link CoreContainer} for this node
*/
public CoreContainer getCoreContainer() {
- if (getSolrDispatchFilter() == null || getSolrDispatchFilter().getCores()
== null) {
+ if (getSolrDispatchFilter() == null) {
Review comment:
I'd still worry about test code that calls this and races before its set.
--
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.
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
