-
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-11613 SAI compressed indexes #1474
base: main
Are you sure you want to change the base?
Changes from all commits
b300ae5
3ceac90
21cd50a
0ea0103
614fe78
f1211a0
e5147cb
14a648c
2ae6972
b0691a2
71a3a1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2118,15 +2118,22 @@ public static void setCommitLogCompression(ParameterizedClass compressor) | |
|
||
public static Config.FlushCompression getFlushCompression() | ||
{ | ||
return conf.flush_compression; | ||
return Objects.requireNonNullElseGet(conf.flush_compression, () -> shouldUseAdaptiveCompressionByDefault() | ||
? Config.FlushCompression.adaptive | ||
: Config.FlushCompression.fast); | ||
} | ||
|
||
public static void setFlushCompression(Config.FlushCompression compression) | ||
{ | ||
conf.flush_compression = compression; | ||
} | ||
|
||
/** | ||
public static boolean shouldUseAdaptiveCompressionByDefault() | ||
{ | ||
return System.getProperty("cassandra.default_sstable_compression", "fast").equals("adaptive"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would probably be better in |
||
} | ||
|
||
/** | ||
* Maximum number of buffers in the compression pool. The default value is 3, it should not be set lower than that | ||
* (one segment in compression, one written to, one in reserve); delays in compression may cause the log to use | ||
* more, depending on how soon the sync policy stops all writing threads. | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -21,6 +21,7 @@ | |||||||||||
import java.util.Map; | ||||||||||||
import java.util.Map.Entry; | ||||||||||||
import java.util.TreeMap; | ||||||||||||
import java.util.function.Consumer; | ||||||||||||
|
||||||||||||
import com.google.common.annotations.VisibleForTesting; | ||||||||||||
|
||||||||||||
|
@@ -238,4 +239,36 @@ public String toString() | |||||||||||
{ | ||||||||||||
return builder.toString(); | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Builds a `WITH option1 = ... AND option2 = ... AND option3 = ... clause | ||||||||||||
* @param builder a receiver to receive a builder allowing to add each option | ||||||||||||
Comment on lines
+244
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
*/ | ||||||||||||
public CqlBuilder appendOptions(Consumer<OptionsBuilder> builder) | ||||||||||||
{ | ||||||||||||
builder.accept(new OptionsBuilder(this)); | ||||||||||||
return this; | ||||||||||||
} | ||||||||||||
|
||||||||||||
public static class OptionsBuilder | ||||||||||||
{ | ||||||||||||
private CqlBuilder builder; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be |
||||||||||||
boolean empty = true; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be |
||||||||||||
|
||||||||||||
OptionsBuilder(CqlBuilder builder) | ||||||||||||
{ | ||||||||||||
this.builder = builder; | ||||||||||||
} | ||||||||||||
|
||||||||||||
public OptionsBuilder append(String name, Map<String, String> options) | ||||||||||||
{ | ||||||||||||
if (options.isEmpty()) | ||||||||||||
return this; | ||||||||||||
|
||||||||||||
builder.append((empty ? " WITH " : " AND ") + name + " = "); | ||||||||||||
empty = false; | ||||||||||||
builder.append(options); | ||||||||||||
return this; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,9 @@ | |
import org.apache.cassandra.audit.AuditLogContext; | ||
import org.apache.cassandra.audit.AuditLogEntryType; | ||
import org.apache.cassandra.auth.Permission; | ||
import org.apache.cassandra.config.CassandraRelevantProperties; | ||
import org.apache.cassandra.config.DatabaseDescriptor; | ||
import org.apache.cassandra.config.ParameterizedClass; | ||
import org.apache.cassandra.cql3.ColumnIdentifier; | ||
import org.apache.cassandra.cql3.QualifiedName; | ||
import org.apache.cassandra.cql3.statements.RawKeyspaceAwareStatement; | ||
|
@@ -161,7 +163,21 @@ public Keyspaces apply(Keyspaces schema) | |
|
||
Map<String, String> options = attrs.isCustom ? attrs.getOptions() : Collections.emptyMap(); | ||
|
||
IndexMetadata index = IndexMetadata.fromIndexTargets(indexTargets, name, kind, options); | ||
Map<String, String> keyCompressionOptions = attrs.getMap("key_compression"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can make |
||
CompressionParams keyCompression = keyCompressionOptions != null | ||
? CompressionParams.fromMap(keyCompressionOptions) | ||
: CompressionParams.noCompression(); | ||
|
||
Map<String, String> valueCompressionOptions = attrs.getMap("value_compression"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, we can make |
||
CompressionParams valueCompression = valueCompressionOptions != null | ||
? CompressionParams.fromMap(valueCompressionOptions) | ||
: CompressionParams.noCompression(); | ||
|
||
if ((keyCompression.isEnabled() || valueCompression.isEnabled()) && !CassandraRelevantProperties.INDEX_COMPRESSION.getBoolean()) | ||
throw ire("Cannot create a compressed index, because index compression is disabled. " + | ||
"Please set " + CassandraRelevantProperties.INDEX_COMPRESSION.getKey() + " property to enable it."); | ||
|
||
IndexMetadata index = IndexMetadata.fromIndexTargets(indexTargets, name, kind, options, keyCompression, valueCompression); | ||
|
||
String className = index.getIndexClassName(); | ||
IndexGuardrails guardRails = IndexGuardrails.forClassName(className); | ||
|
@@ -194,7 +210,10 @@ public Keyspaces apply(Keyspaces schema) | |
throw ire("Index %s is a duplicate of existing index %s", index.name, equalIndex.name); | ||
} | ||
|
||
TableMetadata newTable = table.withSwapped(table.indexes.with(index)); | ||
// All indexes on one table must use the same key_compression. | ||
// The newly created index forces key_compression on the previous indexes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to emit a client warning about this? |
||
Indexes newIndexes = table.indexes.withKeyCompression(index.keyCompression).with(index); | ||
TableMetadata newTable = table.withSwapped(newIndexes); | ||
newTable.validate(); | ||
|
||
return schema.withAddedOrUpdated(keyspace.withSwapped(keyspace.tables.withSwapped(newTable))); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,7 @@ | |
import org.apache.cassandra.io.sstable.format.SSTableReader; | ||
import org.apache.cassandra.io.util.FileUtils; | ||
import org.apache.cassandra.schema.ColumnMetadata; | ||
import org.apache.cassandra.schema.CompressionParams; | ||
import org.apache.cassandra.schema.IndexMetadata; | ||
import org.apache.cassandra.schema.TableId; | ||
import org.apache.cassandra.utils.CloseableIterator; | ||
|
@@ -609,6 +610,16 @@ public String getIndexName() | |
return this.config == null ? null : config.name; | ||
} | ||
|
||
public CompressionParams getKeyCompression() | ||
{ | ||
return this.config.keyCompression; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: unneeded There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method seems unused. |
||
} | ||
|
||
public CompressionParams getValueCompression() | ||
{ | ||
return this.config.valueCompression; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: unneeded |
||
} | ||
|
||
public int getIntOption(String name, int defaultValue) | ||
{ | ||
String value = this.config.options.get(name); | ||
|
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.
Being this a DS-only property, should we use a different prefix, as in
ds.index.compression_enabled
, so it's easier for us to identify these properties?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.
Also, the property could be named
INDEX_COMPRESSION_ENABLED
, or perhapsUSE_INDEX_COMPRESSION
, so the name suggests that it's a boolean property.