SOLR-16855: Add a MigrateReplicas API#1730
Conversation
tflobbe
left a comment
tflobbe
left a comment
There was a problem hiding this comment.
Looks great. Just a couple questions
| if (!clusterState.liveNodesContain(sourceNode)) { | ||
| throw new SolrException( | ||
| SolrException.ErrorCode.BAD_REQUEST, "Source Node: " + sourceNode + " is not live"); | ||
| } |
There was a problem hiding this comment.
Currently the ReplaceNode/BalanceReplicas logic does not allow for moving replicas off of non-live nodes. I think that's something that we should think about for a separate ticket. Especially now that cores are not deleted on startup if they don't exist in Zookeeper. #1321
There was a problem hiding this comment.
I'm not sure how this will work currently. My thinking is, nodes that are removed, you want to "vacate" them and have the replicas be created somewhere else, so that:
- You don't see the node anymore in the "nodes" page and
- You get your replication factor back to whatever it was before
Would this API be the one to use in this case?
There was a problem hiding this comment.
Yeah I do think this is the correct API for that, but it (REPLACENODE) is not currently able to do that. AFAIK.
I think we should make this a param as a part of MigrateReplicas and ReplaceNode. (migrateFromNonLiveNodes or something like that). That would somewhat necessitate solr.deleteUnknownCores, from the above linked PR, to be set to true, so that when the node is started back up, it deletes the data directories from those nodes.
Again, I think this should be done in a separate JIRA/PR.
|
So I found a bug, or room for improvement, in the orderedNodePlacementPlugin code due to one of the tests. Basically it will do a greedy lookup to find the best node for each new replica type. However, an un-optimal ordering of the replicas being placed can result in an un-optimal set of placements. Its hard to make this logic fool-proof, without blowing up the computation time. But I've come up with a "hack", not really, but it's only fixes a subset of the un-optimal cases, that we can have that works for now. We'll need to make the OrderedNodePlacementPlugin smarter to make all cases optimal, or at least closer to optimal. This is better than the previous implementations, which also would have had the same bug. But now we can fix it for all placement plugins at once! |
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
- Remove old WeightedNode sorting logic that is no longer needed - Remove Balancing stashed weight logic that isn't used
|
Ok the tests now pass (fixed a weird bug where splitShard can send a placementRequest with no actual replicas to add... but anyways) The "hacky" fix for tied-nodes, is now expanded and no longer "hacky". We only loop once to retry replicas skipped because of tied nodes, but that should be ok. We can improve this later by looping more if we need to. However, the code is now generalized for any amount of tied-nodes, and will act accordingly given the number of replicas to place and the number of tied nodes. I wish that this fix could have been separate from this PR, but there is little point having this MigrateReplicas API if it isn't going to do a good migration. @tflobbe Amazingly, |
|
I'll merge this early next week in case anyone wants to take a look at the new logic/data structures used for the orderedNodes placement stuff. |
Also: - Fix orderedPlacement logic for tied nodes - Remove stashed weight logic in WeightedNode that is no longer used (cherry picked from commit b3883ad)
2 participants
https://issues.apache.org/jira/browse/SOLR-16855