Skip to content

[SOLR-18114] CloudSolrClient fails deleteById with directUpdatesToLeadersOnly but no route passed#4219

Open
mlbiscoc wants to merge 4 commits intoapache:mainfrom
mlbiscoc:SOLR-18114-deletebyid-bug
Open

[SOLR-18114] CloudSolrClient fails deleteById with directUpdatesToLeadersOnly but no route passed#4219
mlbiscoc wants to merge 4 commits intoapache:mainfrom
mlbiscoc:SOLR-18114-deletebyid-bug

Conversation

@mlbiscoc
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-18114

CloudSolrClient fails when collection has router field configure deleteById and has directUpdatesToLeadersOnly=true but is sent without routing info.

This is because of a bug where hasInfoToFindLeaders() method never actually checked if the deleteById documents had routing info, causing the method to incorrectly return true. This results in a SERVICE_UNAVAILABLE exception instead of falling back to the unoptimized general path to broadcast deletes to all shards.

SolrException.ErrorCode.SERVICE_UNAVAILABLE,
"directUpdatesToLeadersOnly==true but could not find leader(s)");
} else {
// we could not find a leader or routes yet - use unoptimized general path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only ran into this bug because it was forgotten to pass the the routing info on delete. On the flip side, if this exception didn't occur, deletes would have passed and I never would have known deletes we're being sent in an unoptimized way. One way to catch is to add a log here to a SolrJ client but I think that'd be way to noisy. Maybe a debug log? Or maybe that is the correct way and just never log.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is definitely worth a debug log at least; even "info" is defensible since, after all, people use CSC because it will route optimally. So if it can't, it's noteworthy. We can test for this via LogListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're ok with this, then I added a single log.warn message there and added the LogListener test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you one-upped me and went to warn :-) LOL sure, what the heck. But you're on the hook if anyone complains ;-)

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor feedback but I like the idea of a log

return results;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding much-needed javadocs!

.build()) {

UpdateRequest addRequest = new UpdateRequest();
addRequest.add("id", "doc1", "router_field_s", "testRoute");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's pick a doc ID and router_field_s value randomly, unless you need a consistent value for some reason?

@mlbiscoc
Copy link
Contributor Author

Is there something relevant to my PR that won't let SolrJ CI run? I have doubts because it looks like a permission issue.

@dsmiley
Copy link
Contributor

dsmiley commented Mar 21, 2026

Your PR couldn't have anything to do with that. merge anyway. But thanks for bringing it to my attention. It's a chore when sporadically CI hiccups or something, which seems increasingly so. I'll look into this specific matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants