dsmiley commented on code in PR #1027:
URL: https://github.com/apache/solr/pull/1027#discussion_r975682802
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -102,6 +111,19 @@ public void call(ClusterState state, ZkNodeProps message,
NamedList<Object> resu
split(state, message, results);
}
+ /**
+ * Shard splits start here and make additional requests to the host of the
parent shard.
+ *
+ * <p>The sequence of requests is as follows: 1. Verify that there is enough
disk space to create
Review Comment:
This isn't formatted as originally intended.
##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -51,28 +52,41 @@
import org.apache.solr.search.SolrIndexSearcher;
import org.apache.solr.update.SolrIndexSplitter;
import org.apache.solr.update.SplitIndexCommand;
+import org.apache.solr.update.UpdateHandler;
import org.apache.solr.util.RTimer;
import org.apache.solr.util.RefCounted;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * CoreAdminOp implementation for shard splits. This request is enqueued when
{@link SplitShardCmd}
+ * is processed by the Overseer.
Review Comment:
Is "enqueue" the right word -- isn't it just a direct HTTP call, or is there
actually a message in a queue?
I'd drop "is processed by the Overseer".
##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -51,28 +52,41 @@
import org.apache.solr.search.SolrIndexSearcher;
import org.apache.solr.update.SolrIndexSplitter;
import org.apache.solr.update.SplitIndexCommand;
+import org.apache.solr.update.UpdateHandler;
import org.apache.solr.util.RTimer;
import org.apache.solr.util.RefCounted;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * CoreAdminOp implementation for shard splits. This request is enqueued when
{@link SplitShardCmd}
+ * is processed by the Overseer.
+ *
+ * <p>This operation handles two types of requests: 1. If {@link
CommonAdminParams#SPLIT_BY_PREFIX}
+ * is true, the request to calculate document ranges for the sub-shards is
processed here. 2. For
+ * any split request, the actual index split is processed here. This calls
into {@link
+ * UpdateHandler#split(SplitIndexCommand)} to execute split.
+ */
class SplitOp implements CoreAdminHandler.CoreAdminOp {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
@Override
public void execute(CoreAdminHandler.CallInfo it) throws Exception {
SolrParams params = it.req.getParams();
-
+ log.info("Request to SplitOp made with params: {}", params);
Review Comment:
In general, requests with params are already logged; isn't his redundant?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -80,6 +80,15 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Credits to Megan Carey for documentation
+ *
+ * <p>Index split request processed by Overseer. Requests from here go to the
host of the parent
Review Comment:
In Solr 9, the Overseer doesn't necessarily process this; right?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -80,6 +80,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Index split request processed by Overseer. Requests from here go to the
host of the parent shard,
Review Comment:
Not necessarily by the Overseer in Solr 9? Probably not a good idea to say
who one's caller is in general as it can get stale and may not matter.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -80,6 +80,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Index split request processed by Overseer. Requests from here go to the
host of the parent shard,
+ * and are processed by SplitOp.
+ *
+ * <p>There is a shard split doc (dev-docs/shard-split/shard-split.adoc) on
how shard split works;
Review Comment:
I think this is most likely to noticed below where the steps are enumerated.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -80,6 +80,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Index split request processed by Overseer. Requests from here go to the
host of the parent shard,
+ * and are processed by SplitOp.
Review Comment:
```suggestion
* and are processed by {@link SplitOp}.
```
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -162,6 +183,10 @@ public boolean split(ClusterState clusterState,
ZkNodeProps message, NamedList<O
}
RTimerTree t;
+ // 1. verify that there is enough space on disk to create sub-shards
+ log.info(
Review Comment:
Let's move this inside checkDiskSpace since sometimes we don't check the
disk space actually.
##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -124,7 +139,10 @@ public void execute(CoreAdminHandler.CallInfo it) throws
Exception {
DocRouter router = null;
String routeFieldName = null;
+ // if in SolrCloud mode, get collection and shard names
if (it.handler.coreContainer.isZooKeeperAware()) {
+ log.info(
Review Comment:
Feels like `trace` to me
##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -145,6 +163,7 @@ public void execute(CoreAdminHandler.CallInfo it) throws
Exception {
}
if (pathsArr == null) {
+ log.info("SplitOp: Create array of paths for sub-shards of core: {}",
cname);
Review Comment:
Feels like `trace` to me
##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -189,6 +208,7 @@ public void execute(CoreAdminHandler.CallInfo it) throws
Exception {
parentCore.getUpdateHandler().split(cmd);
if (it.handler.coreContainer.isZooKeeperAware()) {
+ log.info("SplitOp: Create cloud descriptors for sub-shards of core:
{}", cname);
Review Comment:
Feels like `trace` to me
--
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]
