Coordinator drop segment selection through cost balancer #5529
Conversation
clintropolis
changed the title
There was a problem hiding this comment.
LGTM overall. I'd like to comment one thing. I'm not sure it's easy to add a test or even it's possible, but it would be great if we can have a unit test to avoid the same issue in the future. @clintropolis what do you think?
…e with cost balancer
| @@ -31,5 +33,10 @@ | |||
|
|
|||
| BalancerSegmentHolder pickSegmentToMove(List<ServerHolder> serverHolders); | |||
|
|
|||
| default Iterator<ServerHolder> pickServersToDrop(DataSegment toDropSegment, NavigableSet<ServerHolder> serverHolders) | |||
| { | |||
| return serverHolders.descendingIterator(); | |||
There was a problem hiding this comment.
Would be nice to include a comment about why this descendingIterator is meaningful. (Like the comment on the old code: // Use the reverse order to get the holders with least available size first.)
| // Use the reverse order to get the holders with least available size first. | ||
| final Iterator<ServerHolder> iterator = holdersInTier.descendingIterator(); | ||
| final NavigableSet<ServerHolder> isServingSubset = | ||
| holdersInTier.stream().filter(s -> s.isServingSegment(segment)).collect(Collectors.toCollection(TreeSet::new)); |
There was a problem hiding this comment.
There's still a (now pointless) check for isServingSegment below. It should be removed lest it confuse people.
There was a problem hiding this comment.
I left the check in case in the time between the initial filtering and the loop something changed and the server was no longer serving the segment, but I can remove the extra check if you think it's safe.
There was a problem hiding this comment.
If there is a race between pickServersToDrop and this line, then there's still a race even with the check. I think it's ok to leave the check in, but maybe log a warning if it actually gets tripped. (We don't expect it to unless there is some race)
| @@ -31,5 +33,10 @@ | |||
|
|
|||
| BalancerSegmentHolder pickSegmentToMove(List<ServerHolder> serverHolders); | |||
|
|
|||
| default Iterator<ServerHolder> pickServersToDrop(DataSegment toDropSegment, NavigableSet<ServerHolder> serverHolders) | |||
There was a problem hiding this comment.
This should have some javadoc explaining what the returned iterator should be. It's non-obvious: the "obvious" return value would be a set of servers to drop from, where the order doesn't matter. But the actual return value is ordered by preferredness, and may contain more servers than we actually want to drop from.
There was a problem hiding this comment.
I went ahead and added javadoc for whole interface, lmk if I got anything wrong
|
|
||
| try { | ||
| List<Pair<Double, ServerHolder>> results = resultsFuture.get(); | ||
| return results.stream() |
There was a problem hiding this comment.
This code is slightly obtuse (what's the double? why reverse it?) and would benefit from a comment.
|
The test that failed, |
* drop selection through cost balancer * use collections.emptyIterator * add test to ensure does not drop from server with larger loading queue with cost balancer * javadocs and comments to clear things up * random drop for completeness
* drop selection through cost balancer * use collections.emptyIterator * add test to ensure does not drop from server with larger loading queue with cost balancer * javadocs and comments to clear things up * random drop for completeness
* drop selection through cost balancer * use collections.emptyIterator * add test to ensure does not drop from server with larger loading queue with cost balancer * javadocs and comments to clear things up * random drop for completeness
* drop selection through cost balancer * use collections.emptyIterator * add test to ensure does not drop from server with larger loading queue with cost balancer * javadocs and comments to clear things up * random drop for completeness
apache#5948) * drop selection through cost balancer * use collections.emptyIterator * add test to ensure does not drop from server with larger loading queue with cost balancer * javadocs and comments to clear things up * random drop for completeness
Fixes issues that occur due to load rule segment drop does not use the segment balancer, one of the issues described in #5521.
Drop segment will now use the balancer to get an iterator of
ServerHolderobjects which can be consumed to get the best candidate servers to drop segments from.