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
ACCUMULO-4187: Added rate limiting for major compactions. #90
Conversation
import java.util.List; | ||
import java.util.WeakHashMap; | ||
import java.util.concurrent.Callable; | ||
import java.util.concurrent.atomic.LongAdder; |
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.
Unfortunately, LongAdder is only available in JDK8, and we're still on JDK7.
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 was unaware of LongAdder... its really neat. Yet another nice thing in JDK8 we can't use yet.
Made a first pass through the code. Wow! Great work for a first contribution @ShawnWalker! Some general themes:
Some new tests on these new classes (testing the rate limiting components and input/output streams should be really important) would really make this even better. I'll have to go back to reread about the use of |
private long currentRate; | ||
|
||
public GuavaRateLimiter(long initialRate) { | ||
this.currentRate = initialRate; |
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.
should there be a sanity check here to ensure non negative? or are there sufficient checks elsewhere?
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'm adopting the convention that a non-positive rate should mean "unlimited", and so allowing non-positive values as the current rate.
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.
The docs for tserver.compaction.major.throughput
specify using 0 for unlimited. Is specifying 0 or negative documented elsewhere?
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.
Looking into the validation associated with, PropertyType.MEMORY it seems to check for >=0.
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.
There's a hint in the javadocs for RateLimiter.getRate()
:
/** Get current QPS of the rate limiter, with a nonpositive rate indicating no limit. */
public long getRate();
But I'll add more explicit comments to GuavaRateLimiter
and to SharedRateLimiterFactory
5139633
to
1303cb8
Compare
I played around with this branch locally. I created a table with 10,000,000 entries using test_ingest using the following commands.
I set the rate limit to 5M and forced a compaction. I saw the following in the tserver logs.
Then I split the table into 8 tablets and forced a compaction to test the rate limit for multiple threads. I had the default of 3 compaction threads. I saw the following in the logs for this test.
|
I've made changes to address most of the comments on this thread. I've also addressed a performance concern that Keith Turner noticed ( I've additionally fixed an issue with tracing in |
@ShawnWalker That sounds great! If you can separate out the issue with ShellServerIT as a separate issue, that'd be helpful. I'm guessing it affects older branches, as well. |
@ShawnWalker I suspect the issue you fixed that was causing ShellServerIT to fail was introduced by ACCUMULO-1755 |
+1 |
8ef536e
to
09dcca6
Compare
I've reverse committed changes to |
I think everything I asked about has been taken care of. Thanks for that, Shawn. The only thing I don't see (and I didn't say it explicitly earlier, so I don't think it's a blocker to merge this in) is a high-level test. I see you added some tests for the rate-limiter piece. I'm wondering if we could make an integration test specifically to test this feature. It's nice when we have a general test class (built around a minicluster) available so that we can easily test potential bugs and add new tests easily. @keith-turner LMK if you have time to merge this in and run the tests. Otherwise, I'll kick off something myself. |
@joshelser I can merge it. An end-to-end test would be nice to detect regressions. I suppose the test would make sure a compaction doesn't run too fast? |
Yeah, I was thinking about how best to test this. You can only reasonably assert a lower-bound on compaction time (to avoid performance skew on certain hosts). Maybe turning off compression for a table, writing a bunch of data and then asserting that a compaction takes at least X time is easiest. You'll still have to account for "compression" from the run-length encoding, but at least that should be uniform across hosts. |
With Keith's help, I've added a small end-to-end IT on rate limiting of major compactions. |
Looks good. Great work guys! 👍 |
Looks like there's a few trivial findbugs issues to address in the IT. |
Added configuration property tserver.compaction.major.throughput of type PropertyType.MEMORY with a default of 0B (unlimited). If another value is specified (e.g. 30M), then all tablet servers will limit the I/O performed during major compaction accordingly (e.g. neither reading nor writing more than 30MiB per second combined over all major compaction threads).
f6adc25
to
783314c
Compare
Added configuration property
tserver.compaction.major.throughput
of typePropertyType.MEMORY
to control rate limiting of major compactions on each tserver.Specifying a value of
0B
(the default) disables rate limiting.If a positive value is specified, then all tablet servers will limit the I/O performed during major compaction accordingly. For example, with
tserver.compaction.major.throughput=30M
, then each tserver will read no more than 30MiB per second and write no more than 30MiB combined over all major compaction threads.This change involved adding an optional
RateLimiter
parameter toFileOperations.openReader(...)
andFileOperations.openWriter(...)
. Most of the file changes involve adding an appropriatenull
to invocations of these methods.