smiklosovic commented on code in PR #2254:
URL: https://github.com/apache/cassandra/pull/2254#discussion_r1176705606


##########
src/java/org/apache/cassandra/schema/CompressionParams.java:
##########
@@ -97,128 +109,185 @@
     // TODO: deprecated, should now be carefully removed. Doesn't affect 
schema code as it isn't included in equals() and hashCode()
     private volatile double crcCheckChance = 1.0;
 
-    public static CompressionParams fromMap(Map<String, String> opts)
-    {
-        Map<String, String> options = copyOptions(opts);
-
-        String sstableCompressionClass;
-
-        if (!opts.isEmpty() && isEnabled(opts) && 
!containsSstableCompressionClass(opts))
-            throw new ConfigurationException(format("Missing sub-option '%s' 
for the 'compression' option.", CLASS));
 
-        if (!removeEnabled(options))
-        {
-            sstableCompressionClass = null;
+    public enum CompressorType
+    {
+        lz4("LZ4Compressor"),
+        none(null),
+        noop("NoopCompressor"),
+        snappy("SnappyCompressor"),
+        deflate("DeflateCompressor"),
+        zstd("ZstdCompressor");
 
-            if (!options.isEmpty())
-                throw new ConfigurationException(format("If the '%s' option is 
set to false no other options must be specified", ENABLED));
-        }
-        else
-        {
-            sstableCompressionClass = removeSstableCompressionClass(options);
+        String className;
+        CompressorType(String className) {
+            this.className = className;
         }
 
-        int chunkLength = removeChunkLength(options);
-        double minCompressRatio = removeMinCompressRatio(options);
-
-        CompressionParams cp = new CompressionParams(sstableCompressionClass, 
options, chunkLength, minCompressRatio);
-        cp.validate();
-
-        return cp;
+        static CompressorType forClass(String name) {
+            for (CompressorType type : CompressorType.values()) {
+                if (Objects.equal(type.className, name)) {
+                    return type;
+                }
+            }
+            return null;
+        }
     }
 
-    public Class<? extends ICompressor> klass()
+    public static CompressionParams defaultParams()
     {
-        return sstableCompressor.getClass();
+        return 
fromParameterizedClass(DatabaseDescriptor.getSSTableCompressionOptions());
     }
 
-    public static CompressionParams noCompression()
+    public static CompressionParams fromParameterizedClass(ParameterizedClass 
options)
     {
-        return new CompressionParams((ICompressor) null, DEFAULT_CHUNK_LENGTH, 
Integer.MAX_VALUE, 0.0, Collections.emptyMap());
-    }
+        if (options == null)
+        {
+            return 
CassandraRelevantProperties.DETERMINISM_SSTABLE_COMPRESSION_DEFAULT.getBoolean()
+                   ? DEFAULT
+                   : noCompression();
+        }
 
-    // The shorthand methods below are only used for tests. They are a little 
inconsistent in their choice of
-    // parameters -- this is done on purpose to test out various compression 
parameter combinations.
+        if (options.parameters != null && 
options.parameters.containsKey(SSTABLE_COMPRESSION))
+            throw new ConfigurationException(format("The '%s' option must not 
be used with the ParameterizedClass constructor",
+                                                    SSTABLE_COMPRESSION));
 
-    @VisibleForTesting
-    public static CompressionParams snappy()
-    {
-        return snappy(DEFAULT_CHUNK_LENGTH);
+        return fromClassAndOptions(options.class_name, options.parameters == 
null ? Collections.emptyMap() : copyOptions(options.parameters));
     }
 
-    @VisibleForTesting
-    public static CompressionParams snappy(int chunkLength)
+    private static String invalidValue( String param, Object value)
     {
-        return snappy(chunkLength, 1.1);
+        return format("Invalid '%s' value for the 'compression' option: %s", 
param, value);
     }
 
-    @VisibleForTesting
-    public static CompressionParams snappy(int chunkLength, double 
minCompressRatio)
-    {
-        return new CompressionParams(SnappyCompressor.instance, chunkLength, 
calcMaxCompressedLength(chunkLength, minCompressRatio), minCompressRatio, 
Collections.emptyMap());
+    private static String invalidValue( String param, String extraText, Object 
value) {
+        return format("Invalid '%s' value for the 'compression' option.  %s: 
%s", param, extraText, value );
     }
 
-    @VisibleForTesting
-    public static CompressionParams deflate()
+    public static CompressionParams fromMap(Map<String, String> opts)
     {
-        return deflate(DEFAULT_CHUNK_LENGTH);
-    }
+        Map<String, String> options = copyOptions(opts);
 
-    @VisibleForTesting
-    public static CompressionParams deflate(int chunkLength)
-    {
-        return new CompressionParams(DeflateCompressor.instance, chunkLength, 
Integer.MAX_VALUE, 0.0, Collections.emptyMap());
-    }
+        String sstableCompressionClass = 
removeSstableCompressionClass(options);
 
-    @VisibleForTesting
-    public static CompressionParams lz4()
-    {
-        return lz4(DEFAULT_CHUNK_LENGTH);
+        return fromClassAndOptions(sstableCompressionClass, options);
     }
 
-    @VisibleForTesting
-    public static CompressionParams lz4(int chunkLength)
+    private static CompressionParams fromClassAndOptions(String 
sstableCompressionClass, Map<String,String> options)
     {
-        return lz4(chunkLength, chunkLength);
-    }
+        boolean enabled = true;
 
-    @VisibleForTesting
-    public static CompressionParams lz4(int chunkLength, int 
maxCompressedLength)
-    {
-        return new 
CompressionParams(LZ4Compressor.create(Collections.emptyMap()), chunkLength, 
maxCompressedLength, calcMinCompressRatio(chunkLength, maxCompressedLength), 
Collections.emptyMap());
-    }
+        if (!removeEnabled(options))
+        {
+            enabled = false;
+        }
 
-    public static CompressionParams zstd()
-    {
-        return zstd(DEFAULT_CHUNK_LENGTH);
+        int chunk_length_in_kb = removeChunkLength(options);
+
+        // figure out how we calculate the max_compressed_length and 
min_compress_ratio
+        if (options.containsKey(MIN_COMPRESS_RATIO)  && 
options.containsKey(MAX_COMPRESSED_LENGTH))
+        {
+            throw new ConfigurationException("Can not specify both 
'min_compress_ratio' and 'max_compressed_length' for the compressor 
parameters.");
+        }
+
+        // calculate the max_compressed_length and min_compress_ratio
+        int max_compressed_length;
+        double min_compress_ratio;
+        String max_compressed_length_str = options.remove( 
MAX_COMPRESSED_LENGTH);
+        if (!StringUtils.isBlank(max_compressed_length_str))
+        {
+            try
+            {
+                max_compressed_length = new 
DataStorageSpec.IntKibibytesBound(max_compressed_length_str).toKibibytes();
+                validateMaxCompressedLength( max_compressed_length, 
chunk_length_in_kb);
+                min_compress_ratio = 
CompressionParams.calcMinCompressRatio(chunk_length_in_kb, 
max_compressed_length);
+            } catch (IllegalArgumentException e) {
+                throw new ConfigurationException(invalidValue( 
MAX_COMPRESSED_LENGTH, e.getMessage(), max_compressed_length_str ));
+            }
+        }
+        else
+        {
+            min_compress_ratio = removeMinCompressRatio(options);
+            validateMinCompressRatio( min_compress_ratio );
+            max_compressed_length =  
CompressionParams.calcMaxCompressedLength(chunk_length_in_kb,min_compress_ratio);
+        }
+
+        // try to set compressor type
+        CompressorType compressorType = 
StringUtils.isEmpty(sstableCompressionClass)?CompressorType.lz4:null;
+        if (compressorType == null)
+        {
+            try
+            {
+                compressorType = 
CompressorType.valueOf(sstableCompressionClass);
+            }
+            catch (IllegalArgumentException expected)
+            {
+                compressorType = 
CompressorType.forClass(sstableCompressionClass);
+            }
+        }
+
+        CompressionParams cp = null;
+        if (compressorType != null)
+        {
+
+            switch (compressorType)
+            {
+                case none:
+                    cp = new CompressionParams((ICompressor) null, 
chunk_length_in_kb, max_compressed_length, min_compress_ratio, options);
+                    break;
+                case lz4:
+                    cp = new 
CompressionParams(enabled?LZ4Compressor.create(options):null, 
chunk_length_in_kb, max_compressed_length, min_compress_ratio, options);
+                    break;
+                case snappy:
+                    cp = new 
CompressionParams(enabled?SnappyCompressor.instance:null, chunk_length_in_kb, 
max_compressed_length, min_compress_ratio, options);

Review Comment:
   shouldnt be here `SnappyCompressor.create(options)` instead of 
`SnappyCompressor.instance`? Same for DeflateCompressor.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to