Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimize CostBalancerStrategy #2910

Merged
merged 3 commits into from
May 5, 2016
Merged

Optimize CostBalancerStrategy #2910

merged 3 commits into from
May 5, 2016

Conversation

nishantmonu51
Copy link
Member

Optimizations to Cost balancer -

  1. Avoid creating thread pool on each chooseBestServer call
  2. Use custom impl for finding gapMillis between two segments rather than using Joda impl which is slow.

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)-

CostBalancerStrategyBenchmark.testBenchmark[0]: [measured 1000 out of 1010 rounds, threads: 1 (sequential)]
 round: 0.02 [+- 0.01], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 7, GC.time: 0.01, time.total: 21.60, time.warmup: 0.29, time.bench: 21.31
CostBalancerStrategyBenchmark.testBenchmark[1]: [measured 1000 out of 1010 rounds, threads: 1 (sequential)]
 round: 0.01 [+- 0.00], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 9, GC.time: 0.01, time.total: 5.93, time.warmup: 0.06, time.bench: 5.87

After -

CostBalancerStrategyBenchmark.testBenchmark[0]: [measured 1000 out of 1010 rounds, threads: 1 (sequential)]
 round: 0.01 [+- 0.00], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 7, GC.time: 0.01, time.total: 8.32, time.warmup: 0.17, time.bench: 8.14
CostBalancerStrategyBenchmark.testBenchmark[1]: [measured 1000 out of 1010 rounds, threads: 1 (sequential)]
 round: 0.00 [+- 0.00], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 9, GC.time: 0.01, time.total: 3.64, time.warmup: 0.04, time.bench: 3.60

Joda gap vs custom timeMillis (10X show improvement)-

CostBalancerStrategyBenchmark.testJodaGap[0]: [measured 1000000 out of 1001000 rounds, threads: 1 (sequential)]
 round: 0.00 [+- 0.00], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 118, GC.time: 0.32, time.total: 31.99, time.warmup: 0.04, time.bench: 31.95
CostBalancerStrategyBenchmark.testBalancerGapMillis[0]: [measured 1000000 out of 1001000 rounds, threads: 1 (sequential)]
 round: 0.00 [+- 0.00], round.block: 0.00 [+- 0.00], round.gc: 0.00 [+- 0.00], GC.calls: 0, GC.time: 0.00, time.total: 3.32, time.warmup: 0.01, time.bench: 3.32

@nishantmonu51 nishantmonu51 added this to the 0.9.1 milestone May 2, 2016
xvrl
xvrl reviewed May 2, 2016
View reviewed changes
server/src/main/java/io/druid/server/coordinator/CostBalancerStrategy.java
} else {
return 0;
}
}

public CostBalancerStrategy(DateTime referenceTimestamp, int threadCount)
Copy link
Member

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.

Copy link
Member Author

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.

@fjy
Copy link
Contributor

fjy commented May 4, 2016

👍

@xvrl
Copy link
Member

xvrl commented May 4, 2016

@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.

@nishantmonu51
Copy link
Member Author

@xvrl made the changes, please have a look again.

@nishantmonu51
Optimize CostBalancerStrategy
5678a76 
Ignore benchmark test in normal run

fix test

review comments

fix compilation

fix test
xvrl
xvrl reviewed May 4, 2016
View reviewed changes
server/src/main/java/io/druid/server/coordinator/CostBalancerStrategyFactory.java
public class CostBalancerStrategyFactory implements BalancerStrategyFactory
{
private final int threadCount;
private final ExecutorService exec;
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

xvrl
xvrl reviewed May 4, 2016
View reviewed changes
server/src/main/java/io/druid/server/coordinator/DruidCoordinator.java
// 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 {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@xvrl
Copy link
Member

xvrl commented May 4, 2016

minor comment #2910 (comment) but 👍 otherwise

@fjy fjy merged commit a2dd57c into apache:master May 5, 2016
@drcrallen drcrallen deleted the optimize-cost-balancer branch May 5, 2016 16:02
nishantmonu51 added a commit to metamx/druid that referenced this pull request May 6, 2016
@nishantmonu51
Optimize CostBalancerStrategy (apache#2910)
01dd8d1 
* Optimize CostBalancerStrategy

Ignore benchmark test in normal run

fix test

review comments

fix compilation

fix test

* review comments

* review comment
@xvrl xvrl mentioned this pull request May 16, 2016
Merged
xvrl pushed a commit to metamx/druid that referenced this pull request May 18, 2016
@nishantmonu51 @xvrl
Optimize CostBalancerStrategy (apache#2910)
4b030f7 
* Optimize CostBalancerStrategy

Ignore benchmark test in normal run

fix test

review comments

fix compilation

fix test

* review comments

* review comment
@xvrl xvrl mentioned this pull request May 18, 2016
Merged
drcrallen added a commit to metamx/druid that referenced this pull request May 18, 2016
@drcrallen
Merge pull request #34 from metamx/balancing-improvements
777d4b8 
Backport balancing improvements apache#2910 apache#2964 apache#2972
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
@navis
apache#2910 Support history file for simple shell
81e0d57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.
3 participants
@nishantmonu51 @fjy @xvrl