[GitHub] [solr] dsmiley commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode

["dsmiley (via GitHub)" <gi...@apache.org>]

dsmiley commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1139576657


##########
solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java:
##########
@@ -73,6 +69,10 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler,
   Stats stats;
   TimeSource timeSource;
 
+  public interface Lock {

Review Comment:
   Perhaps this better belongs in LockTree?



##########
solr/core/src/java/org/apache/solr/cloud/api/collections/DistributedCollectionConfigSetCommandRunner.java:
##########
@@ -164,24 +163,7 @@ public void deleteAllAsyncIds() throws Exception {
 
   /**
    * When {@link org.apache.solr.handler.admin.CollectionsHandler#invokeOperation} does not enqueue
-   * to overseer queue and instead calls this method, this method is expected to do the equivalent
-   * of what Overseer does in {@link
-   * org.apache.solr.cloud.OverseerConfigSetMessageHandler#processMessage}.
-   *
-   * <p>The steps leading to that call in the Overseer execution path are (and the equivalent is
-   * done here):
-   *
-   * <ul>
-   *   <li>{@link org.apache.solr.cloud.OverseerTaskProcessor#run()} gets the message from the ZK
-   *       queue, grabs the corresponding locks (write lock on the config set target of the API
-   *       command and a read lock on the base config set if any - the case for config set creation)
-   *       then executes the command using an executor service (it also checks the asyncId if any is
-   *       specified but async calls are not supported for Config Set API calls).
-   *   <li>In {@link org.apache.solr.cloud.OverseerTaskProcessor}.{@code Runner.run()} (run on an
-   *       executor thread) a call is made to {@link
-   *       org.apache.solr.cloud.OverseerConfigSetMessageHandler#processMessage} which does a few
-   *       checks and calls the appropriate Config Set method.
-   * </ul>
+   * to overseer queue and instead calls this method.

Review Comment:
   The former text was useful, I'd prefer it stay to show why it does what it does (due to legacy Overseer operation).



##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -370,11 +380,12 @@ public void run() {
             if (log.isDebugEnabled()) {
               log.debug(
                   "{}: Get the message id: {} message: {}",
-                  messageHandler.getName(),
+                  this.overseerCollectionMessageHandler.getName(),

Review Comment:
   previously, this referred to simply "messageHandler"; can we keep that?



##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
     this.zkStateReader = zkStateReader;
     this.myId = myId;
     this.stats = stats;
-    this.selector = selector;
+    this.overseerCollectionMessageHandler =

Review Comment:
   or simply messageHandler as it isn't ambiguous?



##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -129,17 +128,19 @@ public String toString() {
 
   private final Object waitLock = new Object();
 
-  protected OverseerMessageHandlerSelector selector;
-
   private OverseerNodePrioritizer prioritizer;
 
   private String thisNode;
 
+  private OverseerCollectionMessageHandler overseerCollectionMessageHandler;

Review Comment:
   I suggest simply calling this "messageHandler" as it isn't ambiguous.



##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
     this.zkStateReader = zkStateReader;
     this.myId = myId;
     this.stats = stats;
-    this.selector = selector;
+    this.overseerCollectionMessageHandler =

Review Comment:
   This needs to be closed.  The test failures are related to this; threads leak from it thus it wasn't closed.



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