You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Claudenw (via GitHub)" <gi...@apache.org> on 2023/03/07 14:03:16 UTC
[GitHub] [cassandra] Claudenw opened a new pull request, #2199: Cassandra-12937 sstable compression
Claudenw opened a new pull request, #2199:
URL: https://github.com/apache/cassandra/pull/2199
Added code to set sstable compression option in the `conf.yaml` file.
Original static `CompressionParms.DEFAULT` configuration moved to `CompressionParams.defaultCompression()`
Added `DatabaseDescriptor.getSstableCompression()` and called it in place of the original `CompressionParams.DEFAULT`
Added test to verify that `CompressionParams` is properly constructed from the `Config.sstable_compressor`
Fixes: CASSANDRA-12937
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129653127
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
+ parameters:
+ - chunk_length_in_kb: '16384'
Review Comment:
that's 16MBs!
and not how we do unit type config settings anymore…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1131325834
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -127,11 +119,50 @@ public static CompressionParams fromMap(Map<String, String> opts)
return cp;
}
+ public static CompressionParams fromParameterizedClass(ParameterizedClass parameterizedClass)
+ {
+ Map<String, String> options = new HashMap<>(parameterizedClass.parameters);
+ options.put( CLASS, parameterizedClass.class_name);
+
+ String sstableCompressionClass;
+
+
+ if (!removeEnabled(options))
+ {
+ sstableCompressionClass = null;
+
+ if (!options.isEmpty())
+ throw new ConfigurationException(format("If the '%s' option is set to false no other options must be specified", ENABLED));
Review Comment:
It was copied from Map<String,String> processing. However, the entire code block has been removed and functionality moved to `SSTableCompressionOptions`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129958472
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
Review Comment:
nit: sstables -> SSTables
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129721624
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
Review Comment:
The class `NoopCompressor` probably does what you want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1139430131
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1134,6 +1137,17 @@ public static void setOverrideLoadConfig(Supplier<Config> loadConfig)
exception
}
+ public enum CompressorType
+ {
+ lz4,
+ none,
+ noop,
+ snappy,
+ deflate,
+ zstd,
+ custom,
+ }
Review Comment:
can we move this into SSTableCompressionOptions ? (or do you foresee using it in multiple places (within this package) ?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-1458332041
please remove the unrelated code formatting first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129959467
##########
src/java/org/apache/cassandra/schema/TableParams.java:
##########
@@ -326,7 +327,7 @@ public void appendCqlTo(CqlBuilder builder, boolean isView)
private SpeculativeRetryPolicy additionalWritePolicy = PercentileSpeculativeRetryPolicy.NINETY_NINE_P;
private CachingParams caching = CachingParams.DEFAULT;
private CompactionParams compaction = CompactionParams.DEFAULT;
- private CompressionParams compression = CompressionParams.DEFAULT;
+ private CompressionParams compression = DatabaseDescriptor.getSstableCompression();
Review Comment:
I think it should be `getSSTableCompression`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Cassandra-12937 sstable compression [cassandra]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-2006267776
No longer current
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1140279753
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1134,6 +1137,17 @@ public static void setOverrideLoadConfig(Supplier<Config> loadConfig)
exception
}
+ public enum CompressorType
+ {
+ lz4,
+ none,
+ noop,
+ snappy,
+ deflate,
+ zstd,
+ custom,
+ }
Review Comment:
yes, I agree, I was planning to do this on merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-1494023177
thank you @Claudenw , I will take a look shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1141797255
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -84,6 +84,7 @@
import org.apache.cassandra.locator.InetAddressAndPort;
import org.apache.cassandra.locator.Replica;
import org.apache.cassandra.locator.SeedProvider;
+import org.apache.cassandra.schema.CompressionParams;
Review Comment:
modified as requested
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1134,6 +1137,17 @@ public static void setOverrideLoadConfig(Supplier<Config> loadConfig)
exception
}
+ public enum CompressorType
+ {
+ lz4,
+ none,
+ noop,
+ snappy,
+ deflate,
+ zstd,
+ custom,
+ }
Review Comment:
moved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1139430131
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1134,6 +1137,17 @@ public static void setOverrideLoadConfig(Supplier<Config> loadConfig)
exception
}
+ public enum CompressorType
+ {
+ lz4,
+ none,
+ noop,
+ snappy,
+ deflate,
+ zstd,
+ custom,
+ }
Review Comment:
can we move this into SSTableCompressionOptions ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129722101
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
+ parameters:
+ - chunk_length_in_kb: '16384'
Review Comment:
I was just using the parameters in the `CompressionParms` class. Are you saying that there should be a different name and we do mapping somewhere? Or should the `CompressionParms` be modified to use a different name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129962053
##########
test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java:
##########
@@ -169,6 +171,21 @@ public void readThresholdsFromConfig()
assertThat(c.row_index_read_size_fail_threshold).isEqualTo(new DataStorageSpec.LongBytesBound(1 << 13, KIBIBYTES));
}
+ @Test
+ public void readSSTableCompressionFromConfig()
+ {
+ Config c = load("test/conf/cassandra.yaml");
+
+ assertNotNull(c.sstable_compressor);
+ assertEquals("org.apache.cassandra.io.compress.LZ4Compressor", c.sstable_compressor.class_name);
+ assertThat(c.sstable_compressor.parameters).containsKey(CompressionParams.CHUNK_LENGTH_IN_KB);
+ assertThat(c.sstable_compressor.parameters).containsKey(CompressionParams.MIN_COMPRESS_RATIO);
+ CompressionParams cp = CompressionParams.fromParameterizedClass(c.sstable_compressor);
+ assertThat(cp.chunkLength()).isEqualTo(16384 * 1024);
+ assertThat(cp.isEnabled()).isTrue();
+ assertThat(cp.getSstableCompressor()).isInstanceOf(org.apache.cassandra.io.compress.LZ4Compressor.class);
Review Comment:
can not you import `LZ4Compressor` ? why does it have to be in FQCN form?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129970390
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -127,11 +119,50 @@ public static CompressionParams fromMap(Map<String, String> opts)
return cp;
}
+ public static CompressionParams fromParameterizedClass(ParameterizedClass parameterizedClass)
+ {
+ Map<String, String> options = new HashMap<>(parameterizedClass.parameters);
+ options.put( CLASS, parameterizedClass.class_name);
+
+ String sstableCompressionClass;
+
+
+ if (!removeEnabled(options))
+ {
+ sstableCompressionClass = null;
+
+ if (!options.isEmpty())
+ throw new ConfigurationException(format("If the '%s' option is set to false no other options must be specified", ENABLED));
Review Comment:
this is interesting. I am curious why there is such condition in place. What is wrong with having parameters in config if the feature is not enabled? This is common pattern in cassandra.yaml, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-1469728528
@michaelsembwever do you have any further comments on this pull request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1139426169
##########
conf/cassandra.yaml:
##########
@@ -1625,6 +1625,25 @@ report_unconfirmed_repaired_data_mismatches: false
# warming of auth caches prior to node completing startup. See CASSANDRA-16958
# auth_cache_warming_enabled: false
+
+#sstable_compressor:
+ # Defines the compression params used to read/write the sstables. If undefined the default is LZ4 with 16kb chunk
+ # length. To disable compression set `class_name: org.apache.cassandra.io.compress.NoopCompressor` or
+ # set the `enable` parameter as `false`
+ #enable=false # defaults to true
+
+ #chunk_length: 16KiB : defaults to 16KiB
+ # min_compress_ratio is only used in snappy and custom implementations.
+ #min_compress_ratio: 0.0 : defaults to 0.0 see CompressionParams.DEFAULT_MIN_COMPRESS_RATIO
+ # type should be one of the `CompressorType` values:
+ # lz4, none, noop, snappy, deflate, zstd, or custom
+ #type: custom
+ # if custom type is specified compressor class and parameters must be provided
+ #compressor:
+ # class_name: org.apache.cassandra.io.compress.LZ4Compressor # the compressor class
+ # parameters:
+ # - param1 : value
+
Review Comment:
would it be better to put this section next to the `flush_compression` setting.
and none of the comments help the user understand that (schema) table-level settings will override this. i think that would help many a new user…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1131318524
##########
test/conf/cassandra.yaml:
##########
@@ -103,3 +103,9 @@ memtable:
class_name: org.apache.cassandra.cql3.validation.operations.CreateTest$InvalidMemtableFactoryMethod
test_invalid_factory_field:
class_name: org.apache.cassandra.cql3.validation.operations.CreateTest$InvalidMemtableFactoryField
+
+sstable_compressor:
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129963711
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -127,11 +119,50 @@ public static CompressionParams fromMap(Map<String, String> opts)
return cp;
}
+ public static CompressionParams fromParameterizedClass(ParameterizedClass parameterizedClass)
+ {
+ Map<String, String> options = new HashMap<>(parameterizedClass.parameters);
+ options.put( CLASS, parameterizedClass.class_name);
Review Comment:
nit: redundant space in `.put( CLASS`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129960997
##########
test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java:
##########
@@ -169,6 +171,21 @@ public void readThresholdsFromConfig()
assertThat(c.row_index_read_size_fail_threshold).isEqualTo(new DataStorageSpec.LongBytesBound(1 << 13, KIBIBYTES));
}
+ @Test
+ public void readSSTableCompressionFromConfig()
+ {
+ Config c = load("test/conf/cassandra.yaml");
+
+ assertNotNull(c.sstable_compressor);
+ assertEquals("org.apache.cassandra.io.compress.LZ4Compressor", c.sstable_compressor.class_name);
Review Comment:
can not use `LZ4Compressor.class.getName()` or similar instead of the explicit string?
##########
test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java:
##########
@@ -169,6 +171,21 @@ public void readThresholdsFromConfig()
assertThat(c.row_index_read_size_fail_threshold).isEqualTo(new DataStorageSpec.LongBytesBound(1 << 13, KIBIBYTES));
}
+ @Test
+ public void readSSTableCompressionFromConfig()
+ {
+ Config c = load("test/conf/cassandra.yaml");
+
+ assertNotNull(c.sstable_compressor);
+ assertEquals("org.apache.cassandra.io.compress.LZ4Compressor", c.sstable_compressor.class_name);
Review Comment:
can not we use `LZ4Compressor.class.getName()` or similar instead of the explicit string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1131319036
##########
test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java:
##########
@@ -169,6 +171,21 @@ public void readThresholdsFromConfig()
assertThat(c.row_index_read_size_fail_threshold).isEqualTo(new DataStorageSpec.LongBytesBound(1 << 13, KIBIBYTES));
}
+ @Test
+ public void readSSTableCompressionFromConfig()
+ {
+ Config c = load("test/conf/cassandra.yaml");
+
+ assertNotNull(c.sstable_compressor);
+ assertEquals("org.apache.cassandra.io.compress.LZ4Compressor", c.sstable_compressor.class_name);
+ assertThat(c.sstable_compressor.parameters).containsKey(CompressionParams.CHUNK_LENGTH_IN_KB);
+ assertThat(c.sstable_compressor.parameters).containsKey(CompressionParams.MIN_COMPRESS_RATIO);
+ CompressionParams cp = CompressionParams.fromParameterizedClass(c.sstable_compressor);
+ assertThat(cp.chunkLength()).isEqualTo(16384 * 1024);
+ assertThat(cp.isEnabled()).isTrue();
+ assertThat(cp.getSstableCompressor()).isInstanceOf(org.apache.cassandra.io.compress.LZ4Compressor.class);
Review Comment:
Changed significantly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1131327104
##########
test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java:
##########
@@ -169,6 +171,21 @@ public void readThresholdsFromConfig()
assertThat(c.row_index_read_size_fail_threshold).isEqualTo(new DataStorageSpec.LongBytesBound(1 << 13, KIBIBYTES));
}
+ @Test
+ public void readSSTableCompressionFromConfig()
+ {
+ Config c = load("test/conf/cassandra.yaml");
+
+ assertNotNull(c.sstable_compressor);
+ assertEquals("org.apache.cassandra.io.compress.LZ4Compressor", c.sstable_compressor.class_name);
Review Comment:
Reworked and moved to `SSTableCompressionOptionsTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129659252
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
Review Comment:
```suggestion
# Defines the compression params used to read/write the sstables. If undefined the default is LZ4 with 16kb chunk length. To disable compression set `class_name: None`
```
And `class_name: None` doesn't look to be implemented yet…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Cassandra-12937 sstable compression [cassandra]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw closed pull request #2199: Cassandra-12937 sstable compression
URL: https://github.com/apache/cassandra/pull/2199
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129958219
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -4398,4 +4404,9 @@ public static void setMinTrackedPartitionTombstoneCount(long value)
{
conf.min_tracked_partition_tombstone_count = value;
}
+
+ public static CompressionParams getSstableCompression() {
Review Comment:
nit: braces on new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129963711
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -127,11 +119,50 @@ public static CompressionParams fromMap(Map<String, String> opts)
return cp;
}
+ public static CompressionParams fromParameterizedClass(ParameterizedClass parameterizedClass)
+ {
+ Map<String, String> options = new HashMap<>(parameterizedClass.parameters);
+ options.put( CLASS, parameterizedClass.class_name);
Review Comment:
nit: redundant space in `.put( CLASS`
also, what if `class_name` is null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1132293255
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
+ parameters:
+ - chunk_length_in_kb: '16384'
Review Comment:
Fixed by adding `SSTableCompressionOptions` to manage the yaml properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-1460208856
@michaelsembwever requested changed made
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1139429951
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -84,6 +84,7 @@
import org.apache.cassandra.locator.InetAddressAndPort;
import org.apache.cassandra.locator.Replica;
import org.apache.cassandra.locator.SeedProvider;
+import org.apache.cassandra.schema.CompressionParams;
Review Comment:
import in the wrong direction (cyclic referencing)
stick with SSTableCompressionOptions in this package, and let the schema package (CompressionParams) import it and do the necessary transformation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1130697405
##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -127,11 +119,50 @@ public static CompressionParams fromMap(Map<String, String> opts)
return cp;
}
+ public static CompressionParams fromParameterizedClass(ParameterizedClass parameterizedClass)
+ {
+ Map<String, String> options = new HashMap<>(parameterizedClass.parameters);
+ options.put( CLASS, parameterizedClass.class_name);
+
+ String sstableCompressionClass;
+
+
+ if (!removeEnabled(options))
+ {
+ sstableCompressionClass = null;
+
+ if (!options.isEmpty())
+ throw new ConfigurationException(format("If the '%s' option is set to false no other options must be specified", ENABLED));
Review Comment:
this is normal for schema config classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1130696745
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
+ parameters:
+ - chunk_length_in_kb: '16384'
Review Comment:
i suggest a separate class. CompressionParms is used for inside the schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129659252
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
Review Comment:
```suggestion
# Defines the compression params used to read/write the sstables. If undefined the default is LZ4 with 16kb chunk length. To disable compression set `class_name: org.apache.cassandra.io.compress.NoopCompressor`
```
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
Review Comment:
true! edited previous comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129839477
##########
conf/cassandra.yaml:
##########
@@ -1805,3 +1805,10 @@ drop_compact_storage_enabled: false
# heartbeat_file: /var/lib/cassandra/data/cassandra-heartbeat
# excluded_keyspaces: # comma separated list of keyspaces to exclude from the check
# excluded_tables: # comma separated list of keyspace.table pairs to exclude from the check
+
+sstable_compressor:
+# Defines the compression params used to read/write the sstables. If undefined the old default is used.
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
+ parameters:
+ - chunk_length_in_kb: '16384'
Review Comment:
not sure about the how. just that we would rather not have untyped settings in yaml now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2199: Cassandra-12937 sstable compression
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#discussion_r1129959756
##########
test/conf/cassandra.yaml:
##########
@@ -103,3 +103,9 @@ memtable:
class_name: org.apache.cassandra.cql3.validation.operations.CreateTest$InvalidMemtableFactoryMethod
test_invalid_factory_field:
class_name: org.apache.cassandra.cql3.validation.operations.CreateTest$InvalidMemtableFactoryField
+
+sstable_compressor:
+ - class_name: org.apache.cassandra.io.compress.LZ4Compressor # the comnpressor class
Review Comment:
typo: comnpressor -> compressor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #2199: Cassandra-12937 sstable compression
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #2199:
URL: https://github.com/apache/cassandra/pull/2199#issuecomment-1490031636
I fixed the issues with the testing and have created a branch against trunk rather than 4.1. Associated pull: https://github.com/apache/cassandra/pull/2254
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org