[GitHub] [solr] dsmiley commented on a change in pull request #180: SOLR-15486: make SolrCoreState.pauseUpdatesAndAwaitInflightRequests logic not SolrCloud specific

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #180:
URL: https://github.com/apache/solr/pull/180#discussion_r675533368



##########
File path: solr/CHANGES.txt
##########
@@ -396,7 +396,8 @@ Bug Fixes
 
 Other Changes
 ---------------------
-(No changes)
+* SOLR-15486: During node shutdown pausing of updates and waiting for in-flight update requests to finish
+  before closing cores is no longer SolrCloud specific. (Christine Poerschke, David Smiley)

Review comment:
       LOL all I did was agree with the idea.  But fine; I'm reviewing because I want to be deserving of my name being there ;-)

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1008,6 +1008,29 @@ public boolean isShutDown() {
     return isShutDown;
   }
 
+  /**

Review comment:
       really minor but I prefer that methods, especially private ones,  _follow_ its callers so that you can follow the code reading down without bouncing back up

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1033,27 +1056,10 @@ public void shutdown() {
       if (isZooKeeperAware()) {
         cancelCoreRecoveries();
         zkSys.zkController.preClose();
-        /*
-         * Pause updates for all cores on this node and wait for all in-flight update requests to finish.
-         * Here, we (slightly) delay leader election so that in-flight update requests succeed and we can preserve consistency.
-         *
-         * Jetty already allows a grace period for in-flight requests to complete and our solr cores, searchers etc
-         * are reference counted to allow for graceful shutdown. So we don't worry about any other kind of requests.
-         *
-         * We do not need to unpause ever because the node is being shut down.
-         */
-        getCores().parallelStream().forEach(solrCore -> {
-          SolrCoreState solrCoreState = solrCore.getSolrCoreState();
-          try {
-            solrCoreState.pauseUpdatesAndAwaitInflightRequests();
-          } catch (TimeoutException e) {
-            log.warn("Timed out waiting for in-flight update requests to complete for core: {}", solrCore.getName());
-          } catch (InterruptedException e) {
-            log.warn("Interrupted while waiting for in-flight update requests to complete for core: {}", solrCore.getName());
-            Thread.currentThread().interrupt();
-          }
-        });
+        pauseUpdatesAndAwaitInflightRequests();

Review comment:
       Couldn't this be moved out of the if/else around SolrCloud and thus call out once?




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