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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
