-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize CostBalancerStrategy #2910
Conversation
} else { | ||
return 0; | ||
} | ||
} | ||
|
||
public CostBalancerStrategy(DateTime referenceTimestamp, int threadCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making strategies closeable, how about we inject the executor service and attach it to the lifecycle? I think it would make things cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially started with that approach, but the number of balancer threads is tied to coordinator dynamic config, so i tied it to the coordinator run loop instead of lifecycle.
👍 |
@nishantmonu51 and I talked offline, he'll try a better approach that doesn't require making the balancing strategy closeable, while keeping the number of threads a dynamic config. |
@xvrl made the changes, please have a look again. |
Ignore benchmark test in normal run fix test review comments fix compilation fix test
ae893cb
to
5678a76
Compare
public class CostBalancerStrategyFactory implements BalancerStrategyFactory | ||
{ | ||
private final int threadCount; | ||
private final ExecutorService exec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly make this one a ListeningExecutorService
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
// Don't read state and run state in the same helper otherwise racy conditions may exist | ||
if (leader && startingLeaderCounter == leaderCounter) { | ||
params = helper.run(params); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to use try with resources with the factory, just in case an exception is thrown earlier. This will also close the factory automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
minor comment #2910 (comment) but 👍 otherwise |
* Optimize CostBalancerStrategy Ignore benchmark test in normal run fix test review comments fix compilation fix test * review comments * review comment
* Optimize CostBalancerStrategy Ignore benchmark test in normal run fix test review comments fix compilation fix test * review comments * review comment
Backport balancing improvements apache#2910 apache#2964 apache#2972
Optimizations to Cost balancer -
These changes speeds up computing segment costs fast by 2X.
Benchmark results -
Before (with shared thread pool, was getting oome in benchmarks with creating new thread pool each time)-
After -
Joda gap vs custom timeMillis (10X show improvement)-