SOLR-16507: Change SplitShardCmd to not use NodeStateProvider #1485
Conversation
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
Outdated
Show resolved
Hide resolved
| cloudManager.request( | ||
| new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params)); | ||
| } catch (Exception e) { | ||
| log.error("Error occurred while checking the disk space of the node"); |
There was a problem hiding this comment.
always propagate the exception!
| if (rsp.getResponse() == null) { | ||
| log.warn("cannot verify information for parent shard leader"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I would love to get rid of this. There will be a response. If it can be shown that there isn't in weird cases, I'd rather improve such weird cases so Solr code can make such guarantees without needless/verbose null checks.
You could not declare the "rsp" even; just immediately grab the all-important NamedList.
|
Looking good! I think a net improvement. After this change, NodeStateProvider and its related weird named things (Snitches) is only used in one place. WDYT @sigram ? (BTW I tagged you for review on another unrelated PR by accident) |
|
LGTM , |
There was a problem hiding this comment.
@noblepaul if you have recommendations on how to proceed with simplifying NodeStateProvider / removing Snitch, then please share in the JIRA issue as there's a conversation there already.
I tested this PR locally; looks good. Thanks Vinayak!
I'm going to merge this into main with a CHANGES.txt for the next release in the "Other" section something simple like this:
- SOLR-16507: Change SplitShardCmd to not use NodeStateProvider (Vinayak Hegde, David Smiley)
And we can add onto that entry with another PR that does more for the same issue.
…k Hegde, David Smiley)
I shall submit a PR for the JIRA |
dsmiley
changed the title
use metric API instead of NodeStateProvider in SplitShardCmd.