You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/09/08 15:56:48 UTC
[GitHub] [solr] makosten opened a new pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
makosten opened a new pull request #288:
URL: https://github.com/apache/solr/pull/288
https://issues.apache.org/jira/browse/SOLR-6910
# Description
This is an implementation of distributed delete-by id-when using the CompositeId router and the route field is missing. This is requested by SOLR-6910 and referenced in SOLR-5810 and SOLR-8889. The current behavior in this circumstance is to perform the delete by id only on the shard which received the request, even if the document is on a different shard.
There has been some discussion of instead implementing a strict mode, in which case the delete-by-id would fail with a message that the routing key is required, but these two approaches are not mutually exclusive. This is this "non-strict" implementation. There are use cases for this feature, such as a volatile shard key or implementing a separate indexing pipeline for deletions without consulting a database.
Using delete-by-query is a workaround but is much less performant.
# Solution
This first approach detects this condition and forwards the request to the doDeleteByQuery method. This works, but it would be better to extract the shared code from doDeleteByQuery. I looked at this but need some advice on how to proceed.
# Tests
I've extended an existing test to verify the new behavior.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
[GitHub] [solr] epugh commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r705673077
##########
File path: solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java
##########
@@ -298,6 +303,29 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
docCountsAfrica.decrementAndGet();
}
checkShardCounts.run();
+
+ // Tests for distributing delete by id when route is missing from the request
+
+ { // Send a delete request with no route to shard1 for document on shard2, should be distributed
Review comment:
awesome! I mis-read the patch file!
--
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
[GitHub] [solr] dsmiley commented on pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-944654502
Sorry, I find having `doDeleteById` call `doDeleteByQuery` to be way too kludgy (and you admitted as much in JIRA). It appears that this works almost accidentally, and is thus fragile and perhaps internally slower than it could be. It seems the logic should go into `doDistribDeleteById`.
But firstly, I'm confused about how this PR relates to the JIRA issue. The JIRA issue clearly states this issue occurs when the implicit router is being used. Yet the test here and the fix use/check-for the CompositeIdRouter. Am I missing something?
--
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
[GitHub] [solr] epugh commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r704608615
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -325,6 +325,24 @@ protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
@Override
protected void doDistribDeleteById(DeleteUpdateCommand cmd) throws IOException {
+
+ zkCheck();
Review comment:
This is more of a comment, why is this called zkCheck? icky name.
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-961186594
On the unintentional impact to ImplicitIdRouter, I was thinking adding a property to the router as whether to do the broadcast to avoid isinstanceof CompositeIdRouter. But, there are other places in DistributedZkUpdateProcessor that do exactly that.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743873466
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
I’m LiftBot and I automatically analyze new code in code review, and comment when I find potential bugs. I also accept comments as commands. Just `@sonatype-lift` followed by the command: **ignore** to mark as false positive, **unignore** to undo, and **help** to see this message. [Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.
--
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
[GitHub] [solr] dsmiley commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-950212191
> I've added some logic in the doDistribDeleteById for Phase=NONE to also distribute to request to the leaders of the other shards if the route is missing, which is more along the lines of David Smiley's suggestion.
If you do that and the test fails (presumably), I could help debug to see why.
In terms of reviewers, I think @hossman would be a good choice based on him touching the same test very recently in SOLR-8889 to fix similar-ish bugs.
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-951088242
Thanks @makosten for the diagrams to illustrate process flow, and @epugh for tagging me, this is an intriguing area of the code!
> ... distribute to request to the leaders of the other shards if the route is missing ...
That seems intuitively right. From what I've learnt so far from code reading, the issue is that https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.10.1/solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java#L609 picks a shard even if it does not have sufficient information to do so i.e. in the absence of the route it picks based on id which statistically will only sometimes pick the right shard and distribution to all shard leaders is necessary to be sure it reaches the right shard.
> ... If you do that and the test fails (presumably), I could help debug to see why. ...
I'd be interested too in looking more into this.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743873466
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
I’m LiftBot and I automatically analyze new code in code review, and comment when I find potential bugs. I also accept comments as commands. Just `@sonatype-lift` followed by the command: **ignore** to mark as false positive, **unignore** to undo, and **help** to see this message. [Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
I’m LiftBot and I automatically analyze new code in code review, and comment when I find potential bugs. I also accept comments as commands. Just `@sonatype-lift` followed by the command: **ignore** to mark as false positive, **unignore** to undo, and **help** to see this message. [Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-935165955
I revised the logic to call DeleteByQuery earlier. Now instead of calling doDeleteByQuery frrom doDistribDeleteById, it is called from doDeleteById. I tried revising doDeleteById to forward the request to all shard leaders if the required route is missing from the request, but this did not work. At one time I believe a deleteById would work even if the route was missing if it happened to land on the right shard, but it seems that this behavior is changed.
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-947064112
As far as implicit vs. composite router, you aren't missing anything. I was trying to avoid creating a duplicate JIRA and messed up by picking 6910. The patch only applies to the composite id router. Maybe the best way to unravel this is to create a new JIRA and reference 6910?
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743873440
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
@sonatype-lift help
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-962098372
Added two commits with small tweaks, yet to complete running full test suite locally but assuming it passes LGTM here.
@makosten - would you like to go add a `solr/CHANGES.txt` entry also?
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743873440
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
@sonatype-lift help
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743107726
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
I added creating the rollup replication tracker before forwarding the delete-by-id to the other shard leaders and saw no behavior difference. The handleReplicationFactor method didn't receive responses from other shard leaders.
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
That worked quite nicely!
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
Wrapping the log.debug with an isEnabled fixed this.
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743107726
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
I added creating the rollup replication tracker before forwarding the delete-by-id to the other shard leaders and saw no behavior difference. The handleReplicationFactor method didn't receive responses from other shard leaders.
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
That worked quite nicely!
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
Wrapping the log.debug with an isEnabled fixed this.
--
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
[GitHub] [solr] epugh commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r723637131
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -314,6 +314,23 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
@Override
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
+
+ // if using the CompositeId router and route field is missing, distribute to all shard leaders
+ if(cmd.getRoute()==null) {
+ zkCheck();
+ DocCollection coll = zkController.getClusterState().getCollection(collection);
+ DocRouter router = coll.getRouter();
+ String routeField = router.getRouteField(coll);
+ if (router instanceof CompositeIdRouter && routeField != null) {
+ DistribPhase phase = DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM));
+ if (phase == DistribPhase.NONE) {
+ log.debug("Using compositeId router and deleteById command is with missing route value, distributing to all shard leaders");
Review comment:
@makosten small nit, should this debug line be "command is missing" instead of "is with" ;-)
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743107726
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
I added creating the rollup replication tracker before forwarding the delete-by-id to the other shard leaders and saw no behavior difference. The handleReplicationFactor method didn't receive responses from other shard leaders.
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743313048
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
Wrapping the log.debug with an isEnabled fixed this.
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-962098372
Added two commits with small tweaks, yet to complete running full test suite locally but assuming it passes LGTM here.
@makosten - would you like to go add a `solr/CHANGES.txt` entry also?
--
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
[GitHub] [solr] cpoerschke commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742125759
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
Review comment:
2/4 - if the "just one slice" logic happened earlier so that `broadcastDeleteById` is only true if there's more than one slice this:
* would remove the need for this `if` here
* could facilitate code sharing with `doDeleteByQuery`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
Review comment:
1/4 - minor: maybe include `cmd.getId()` in the debug logging here
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
3/4 - added a commit to the pull request branch -- feel free to revert or amend -- that factors out `forwardDelete` method from `doDeleteByQuery`, for potential code sharing with `doDeleteById`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
4/4 - `doDeleteByQuery` does rollup replication tracker logic before the forward-delete logic, `doDeleteById` here does check replication tracker logic after the forward-logic -- i've not yet considered this difference in detail, just noticed it whilst factoring out the forward-delete logic
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r739917181
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `DistributedZkUpdateProcessor.doDeleteById(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742983766
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
Review comment:
Good idea, I'll make this change.
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
Review comment:
Another good idea. I moved the check to setupRequest(). I believe the most efficient check that return the same value is coll.getActiveSlicesMap().size().
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
This is great! I'm going to see if forwardDelete can be shared.
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742984104
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
I'm assuming this is because of the log.debug()? Should I be concerned?
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-962098372
Added two commits with small tweaks, yet to complete running full test suite locally but assuming it passes LGTM here.
@makosten - would you like to go add a `solr/CHANGES.txt` entry also?
--
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
[GitHub] [solr] epugh commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-950141379
@cpoerschke any chance you'd be interested in reviewing this fix as well? Looking to get some more experienced folks to confirm this patch!
--
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
[GitHub] [solr] epugh commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r704607164
##########
File path: solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java
##########
@@ -298,6 +303,29 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
docCountsAfrica.decrementAndGet();
}
checkShardCounts.run();
+
+ // Tests for distributing delete by id when route is missing from the request
+
+ { // Send a delete request with no route to shard1 for document on shard2, should be distributed
Review comment:
This is a "unique" technique, that I haven't seen before in our code base. I can't really evaluate if it's good or not... Do we do this elsewhere in the Solr code base?
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r704901284
##########
File path: solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java
##########
@@ -298,6 +303,29 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
docCountsAfrica.decrementAndGet();
}
checkShardCounts.run();
+
+ // Tests for distributing delete by id when route is missing from the request
+
+ { // Send a delete request with no route to shard1 for document on shard2, should be distributed
Review comment:
In this case I'm copying the style of the existing test I'm modifying. Code blocks outside of flow-of-control are just for variable scoping, so you can include `final UpdateRequest deleteRequest = new UpdateRequest();` repeatedly.
--
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
[GitHub] [solr] epugh commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r705672767
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -325,6 +325,24 @@ protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
@Override
protected void doDistribDeleteById(DeleteUpdateCommand cmd) throws IOException {
+
+ zkCheck();
Review comment:
Thanks... I am tempted to create a JIRA but..... Will probably just live iwth it.
--
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
[GitHub] [solr] dsmiley commented on pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-947149976
Yes, please create a separate JIRA as it's only vaguely related to SOLR-6910.
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-950160336
Here's a diagram if DeleteById with a compositeId router and a route value:
I've added some logic in the doDistribDeleteById for Phase=NONE to also distribute to request to the leaders of the other shards if the route is missing, which is more along the lines of David Smiley's suggestion. I can now see the local deletion happening on all the nodes, all the way down to DirectUpdateHandler2.delete, but the deletion doesn't happen. This isn't completely surprising, because even now if the receiving node is the shard leader of the correct shard and the route is missing, the local delete still appears to happen on the shard leader and follower but ultimately the deletion fails without error, at least in my test it does. I think if I could discern why it still fails even though the local deletion appears to be happening, that this would be the correct approach.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742198500
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `DistributedZkUpdateProcessor.doDeleteByQuery(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-959652732
> This also affected the Implicit Router as it will now broadcast the delete-by-id request if the route is missing, so I updated a failing test. I'd like some feedback on this, as maybe the old behavior is more desirable for the Implicit router.
From the _"... does not automatically route ..."_ mention at https://solr.apache.org/guide/8_10/collection-management.html#create-parameters documentation I think the existing behaviour for the implicit router makes sense i.e. routing of both indexing and delete-by-id requests is the caller's responsibility.
(If we wanted to change that behaviour, hypothetically, then doing so separately (e.g. as a follow-up issue and pull request) might be clearest and non-backwards compatibility would need to be considered and communicated e.g. users currently doing round-robins on delete-by-id external to Solr can cease doing that and users which use the implicit routing to (temporarily?) have the same document id on multiple shards would benefit from a way to opt-out of the delete-by-id broadcasting.)
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-959652732
--
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
[GitHub] [solr] cpoerschke commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-959652732
> This also affected the Implicit Router as it will now broadcast the delete-by-id request if the route is missing, so I updated a failing test. I'd like some feedback on this, as maybe the old behavior is more desirable for the Implicit router.
From the _"... does not automatically route ..."_ mention at https://solr.apache.org/guide/8_10/collection-management.html#create-parameters documentation I think the existing behaviour for the implicit router makes sense i.e. routing of both indexing and delete-by-id requests is the caller's responsibility.
(If we wanted to change that behaviour, hypothetically, then doing so separately (e.g. as a follow-up issue and pull request) might be clearest and non-backwards compatibility would need to be considered and communicated e.g. users currently doing round-robins on delete-by-id external to Solr can cease doing that and users which use the implicit routing to (temporarily?) have the same document id on multiple shards would benefit from a way to opt-out of the delete-by-id broadcasting.)
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-947905418
Here is a process flow of a DeleteByQuery. The doDistribDeleteByQuery is where a leader forwards the command to its replicas. The doDeleteByQuery in DistributedZkUpdateProcessor is where the command is forwarded to the shard leaders, so I do think that my logic of forwarding the request to doDeleteByQuery is in the right location, although the whole approach is not optimal. I did try modeling how DeleteByQuery works by forwarding the request to shard leaders, but found that the local deleteById, actually deleting the document from the index, does not happen if the route field value is missing.
--
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
[GitHub] [solr] makosten commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-955826088
@cpoerschke You nailed it! The CompositeId Router has some inconsistent behavior. It's strict on the router field when adding but if the route is missing when deleting by id, it returns the slice based on the unique id hash. I updated the CompositeIdRouter to return null for getTargetSlice for this condition, in which case it is run on the current slice, and then flag the request to be broadcast to the other shard leaders.
This also affected the Implicit Router as it will now broadcast the delete-by-id request if the route is missing, so I updated a failing test. I'd like some feedback on this, as maybe the old behavior is more desirable for the Implicit router.
--
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
[GitHub] [solr] cpoerschke commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742125759
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
Review comment:
2/4 - if the "just one slice" logic happened earlier so that `broadcastDeleteById` is only true if there's more than one slice this:
* would remove the need for this `if` here
* could facilitate code sharing with `doDeleteByQuery`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
Review comment:
1/4 - minor: maybe include `cmd.getId()` in the debug logging here
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
3/4 - added a commit to the pull request branch -- feel free to revert or amend -- that factors out `forwardDelete` method from `doDeleteByQuery`, for potential code sharing with `doDeleteById`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
4/4 - `doDeleteByQuery` does rollup replication tracker logic before the forward-delete logic, `doDeleteById` here does check replication tracker logic after the forward-logic -- i've not yet considered this difference in detail, just noticed it whilst factoring out the forward-delete logic
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
Review comment:
2/4 - if the "just one slice" logic happened earlier so that `broadcastDeleteById` is only true if there's more than one slice this:
* would remove the need for this `if` here
* could facilitate code sharing with `doDeleteByQuery`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
Review comment:
1/4 - minor: maybe include `cmd.getId()` in the debug logging here
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
3/4 - added a commit to the pull request branch -- feel free to revert or amend -- that factors out `forwardDelete` method from `doDeleteByQuery`, for potential code sharing with `doDeleteById`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
4/4 - `doDeleteByQuery` does rollup replication tracker logic before the forward-delete logic, `doDeleteById` here does check replication tracker logic after the forward-logic -- i've not yet considered this difference in detail, just noticed it whilst factoring out the forward-delete logic
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742198500
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `DistributedZkUpdateProcessor.doDeleteByQuery(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `DistributedZkUpdateProcessor.doDeleteByQuery(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743151985
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
That worked quite nicely!
--
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
[GitHub] [solr] epugh commented on pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #288:
URL: https://github.com/apache/solr/pull/288#issuecomment-961198543
Thanks @cpoerschke for digging into this PR ;-)
--
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
[GitHub] [solr] makosten commented on a change in pull request #288: SOLR-6910: A deleteById request without _route_ param for implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
makosten commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r704903831
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -325,6 +325,24 @@ protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
@Override
protected void doDistribDeleteById(DeleteUpdateCommand cmd) throws IOException {
+
+ zkCheck();
Review comment:
It verifies that zookeeper (zk) is available.
--
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
[GitHub] [solr] epugh merged pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
epugh merged pull request #288:
URL: https://github.com/apache/solr/pull/288
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742198500
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `DistributedZkUpdateProcessor.doDeleteByQuery(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] cpoerschke commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid or implicit router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r742125759
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
Review comment:
2/4 - if the "just one slice" logic happened earlier so that `broadcastDeleteById` is only true if there's more than one slice this:
* would remove the need for this `if` here
* could facilitate code sharing with `doDeleteByQuery`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
Review comment:
1/4 - minor: maybe include `cmd.getId()` in the debug logging here
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -383,47 +419,7 @@ protected void doDeleteByQuery(DeleteUpdateCommand cmd) throws IOException {
if (rollupReplicationTracker == null) {
rollupReplicationTracker = new RollupRequestReplicationTracker();
}
- boolean leaderForAnyShard = false; // start off by assuming we are not a leader for any shard
-
- ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
- outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
- outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
- zkController.getBaseUrl(), req.getCore().getName()));
-
- SolrParams params = req.getParams();
- String route = params.get(ShardParams._ROUTE_);
- Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
-
- List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size());
- for (Slice slice : slices) {
- String sliceName = slice.getName();
- Replica leader;
- try {
- leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
- } catch (InterruptedException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
- }
-
- // TODO: What if leaders changed in the meantime?
- // should we send out slice-at-a-time and if a node returns "hey, I'm not a leader" (or we get an error because it went down) then look up the new leader?
-
- // Am I the leader for this slice?
- ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
- String leaderCoreNodeName = leader.getName();
- String coreNodeName = cloudDesc.getCoreNodeName();
- isLeader = coreNodeName.equals(leaderCoreNodeName);
-
- if (isLeader) {
- // don't forward to ourself
- leaderForAnyShard = true;
- } else {
- leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
- }
- }
-
- outParams.remove("commit"); // this will be distributed from the local commit
-
- cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ boolean leaderForAnyShard = forwardDelete(coll, cmd);
Review comment:
3/4 - added a commit to the pull request branch -- feel free to revert or amend -- that factors out `forwardDelete` method from `doDeleteByQuery`, for potential code sharing with `doDeleteById`
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
+ if (broadcastDeleteById && DistribPhase.parseParam(req.getParams().get(DISTRIB_UPDATE_PARAM)) == DistribPhase.NONE ) {
+
+ log.debug("The deleteById command is missing the required route, broadcasting to leaders of other shards");
+
+ ModifiableSolrParams outParams = new ModifiableSolrParams(filterParams(req.getParams()));
+ outParams.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString());
+ outParams.set(DISTRIB_FROM, ZkCoreNodeProps.getCoreUrl(
+ zkController.getBaseUrl(), req.getCore().getName()));
+
+ SolrParams params = req.getParams();
+ String route = params.get(ShardParams._ROUTE_);
+ DocCollection coll = clusterState.getCollection(collection);
+ Collection<Slice> slices = coll.getRouter().getSearchSlices(route, params, coll);
+
+ // if just one slice, we can skip this
+ if (slices.size() > 1) {
+ List<SolrCmdDistributor.Node> leaders = new ArrayList<>(slices.size() - 1);
+ for (Slice slice : slices) {
+ String sliceName = slice.getName();
+ if (!sliceName.equals(cloudDesc.getShardId())) {
+ Replica leader;
+ try {
+ leader = zkController.getZkStateReader().getLeaderRetry(collection, sliceName);
+ } catch (InterruptedException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Exception finding leader for shard " + sliceName, e);
+ }
+ ZkCoreNodeProps coreLeaderProps = new ZkCoreNodeProps(leader);
+ leaders.add(new SolrCmdDistributor.ForwardNode(coreLeaderProps, zkController.getZkStateReader(), collection, sliceName, maxRetriesOnForward));
+ }
+ }
+ outParams.remove("commit"); // this will be distributed from the local commit
+ cmdDistrib.distribDelete(cmd, leaders, outParams, false, rollupReplicationTracker, null);
+ }
+ }
+
// check if client has requested minimum replication factor information. will set replicationTracker to null if
// we aren't the leader or subShardLeader
checkReplicationTracker(cmd);
Review comment:
4/4 - `doDeleteByQuery` does rollup replication tracker logic before the forward-delete logic, `doDeleteById` here does check replication tracker logic after the forward-logic -- i've not yet considered this difference in detail, just noticed it whilst factoring out the forward-delete logic
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #288: SOLR-15705: A deleteById request without _route_ param for compositeid router could be sent to all shard leaders
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #288:
URL: https://github.com/apache/solr/pull/288#discussion_r743873440
##########
File path: solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java
##########
@@ -316,6 +317,41 @@ public void processDelete(DeleteUpdateCommand cmd) throws IOException {
protected void doDeleteById(DeleteUpdateCommand cmd) throws IOException {
setupRequest(cmd);
Review comment:
@sonatype-lift help
--
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