-
Notifications
You must be signed in to change notification settings - Fork 22
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
CNDB-11532: Adaptive compression #1432
base: main
Are you sure you want to change the base?
Conversation
DEFAULT_MIN_COMPRESS_RATIO, | ||
Collections.emptyMap()); | ||
|
||
public static final CompressionParams FAST_ADAPTIVE = new CompressionParams(AdaptiveCompressor.createForFlush(Collections.emptyMap()), |
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 not very happy with this name, and generally with the use of "FAST" for "FLUSH" and "GENERAL" for "COMPACTION", but we can't change the enum at this point, can we?
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.
Im not happy either... However if I change the naming, then we should change it for all the things consistently and this is a part of the config (we use "fast" in cassandra.yaml). :(
src/java/org/apache/cassandra/io/compress/AdaptiveCompressor.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/io/compress/AdaptiveCompressor.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/io/compress/AdaptiveCompressor.java
Outdated
Show resolved
Hide resolved
@Override | ||
public Set<Uses> recommendedUses() | ||
{ | ||
return Set.of(params.uses); |
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.
This can use EnumSet.of
.
Further, IMHO it makes better sense to return both here; or rather, add a new method to adapt the compressor for a certain use, e.g.
default ICompressor forUse(Uses use)
{
return recommendedUses.contains(use) ? this : null;
}
in ICompressor
, overridden to change params and recreate in AdaptiveCompressor
.
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.
Actually, such a change is also necessary to preserve encryption for HCD.
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.
+1 to EnumSet.of
However, I don't agree with returning both uses here.
This is because a particular AdaptiveCompressor
can be either adapting to "flush pressure" or "compaction pressure" but not both. I don't want to accidentally end up using AdaptiveCompressor
configured for compaction (aka GENERAL use) in flush, so it cannot advertise as FAST_COMPRESSION unless configured for flushing.
The use is locked at the moment of creating the compressor. It could be actually two separate compressor classes, each for different use, but because they share like 99% of logic and the only real difference is defaults + pressure source, there is likely no point in separating them.
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.
But on the second thought, indeed, if someone configures flushCompression as table
and then AdaptiveCompression
is selected in the table schema... this would result in using a wrong source of pressure. So adding this forUse
API looks like a good idea to me.
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.
This is precisely why I described an adaptation method: we do want to use a compressor derived from the current one, suitable for the flush usage. This can be done without requiring any changes to existing ICompressor
implementations via the default method above, and is important for this patch, but also critical for preserving encryption when DSE moves to this code base.
1806608
to
9bdc785
Compare
current = average.get(); | ||
|
||
if (!Double.isNaN(current)) | ||
update = current + Math.pow(alpha, n) * (val - current); |
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.
This doesn't appear to agree with the javadoc. The effect of val
should be stronger for higher n
, not weaker.
E.g. I believe calling update(val)
twice results in (1-alpha)^2 * current + (1-(1-alpha)^2) * val
, which is not the same as (1 - alpha^2) * current + alpha^2 * val
, which this translates to.
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.
Right. I got it reversed.
* also measures and exposes the actual rate. | ||
*/ | ||
@SuppressWarnings("UnstableApiUsage") | ||
public class RateLimiter |
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.
It would be better to port over CASSANDRA-13890 which introduces a compaction metric for the current compaction rate (with a few aggregations, e.g. 1-minute rate).
787932c
to
f2bfb36
Compare
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 curious why not port over the few other changes in CASSANDRA-13890. Has the code diverged too much?
f2bfb36
to
7222496
Compare
Will take a look. The merge wasn't clean and I didn't initially want to spend time on something that is not essential for this feature. |
This commit introduces a new AdaptiveCompressor class. AdaptiveCompressor uses ZStandard compression with a dynamic compression level based on the current write load. AdaptiveCompressor's goal is to provide similar write performance as LZ4Compressor for write heavy workloads, but a significantly better compression ratio for databases with a moderate amount of writes or on systems with a lot of spare CPU power. If the memtable flush queue builds up, and it turns out the compression is a significant bottleneck, then the compression level used for flushing is decreased to gain speed. Similarly, when pending compaction tasks build up, then the compression level used for compaction is decreased. In order to enable adaptive compression: - set `-Dcassandra.default_sstable_compression=adaptive` JVM option to automatically select `AdaptiveCompressor` as the main compressor for flushes and new tables, if not overriden by specific options in cassandra.yaml or table schema - set `flush_compression: adaptive` in cassandra.yaml to enable it for flushing - set `AdaptiveCompressor` in Table options to enable it for compaction Caution: this feature is not turned on by default because it may impact read speed negatively in some rare cases. Fixes riptano/cndb#11532
7222496
to
b78ed3e
Compare
Quality Gate failedFailed conditions |
❌ Build ds-cassandra-pr-gate/PR-1432 rejected by Butler10 new test failure(s) in 12 builds Found 10 new test failures
Found 110 known test failures |
This commit introduces a new AdaptiveCompressor class.
AdaptiveCompressor uses ZStandard compression with a dynamic compression level based on the current write load. AdaptiveCompressor's goal is to provide similar write performance as LZ4Compressor for write heavy workloads, but a significantly better compression ratio for databases with a moderate amount of writes or on systems with a lot of spare CPU power.
If the memtable flush queue builds up, and it turns out the compression is a significant bottleneck, then the compression level used for flushing is decreased to gain speed. Similarly, when pending compaction tasks build up, then the compression level used for compaction is decreased.
In order to enable adaptive compression:
-Dcassandra.default_sstable_compression=adaptive
JVM option to automatically selectAdaptiveCompressor
as the main compressor for flushes and new tables, if not overriden by specific options in cassandra.yaml or table schemaflush_compression: adaptive
in cassandra.yaml to enable it for flushingAdaptiveCompressor
in Table options to enable it for compactionCaution: this feature is not turned on by default because it
may impact read speed negatively in some rare cases.
Checklist before you submit for review
NoSpamLogger
for log lines that may appear frequently in the logs