dsmiley commented on a change in pull request #155:
URL: https://github.com/apache/solr/pull/155#discussion_r655848769
##########
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:
The line of code change here was a simplification that should work the
same way as before. If SDF.getCores() is null, then the line below will return
null just the same ;-)
##########
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:
I'm just calling Jetty's FilterHolder.stop() which knows the lifecycle
(i.e. won't stop twice). I don't see any interruption code in there.
##########
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:
I removed the metricManager field. Maybe should do the same for
httpClient -- always fetch it on-demand. Fewer fields makes a class easier to
reason about, especially when the lifecycle is governed by other fields!
##########
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:
Next time please add more code comments to justify the complexity you
added so that mortals like myself know why it's there.
A background close thread was added 11/29/2018 but you commented it out 2
days later, leaving `customThreadPool` and some commented code even though it's
not used anymore. Ugh. So I'm unsure how to reconcile the current state with
your explanation above. You explain why a background thread is needed to close
(interesting) but there isn't one today and hasn't been there for years. 🤷
##########
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:
Didn't seem slow when I looked at the code some weeks ago. If I'm
wrong, then I'd propose that our tests have you opt-in to such so you don't pay
for features you don't plan to use.
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
