You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Maxwell-Guo (via GitHub)" <gi...@apache.org> on 2023/11/08 08:28:06 UTC
[PR] Move format-specific sstable options into the format configuration [cassandra]
Maxwell-Guo opened a new pull request, #2879:
URL: https://github.com/apache/cassandra/pull/2879
```
Move format-specific sstable options into the format configuration
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-18533
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413975798
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
I don't want to overthink it, especially as there is only one format that uses these at the moment -- if there are multiple and they don't match, we should raise an exception during startup / the validation of the yaml.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413682788
##########
src/java/org/apache/cassandra/io/sstable/format/big/BigFormatPartitionWriter.java:
##########
@@ -70,7 +70,12 @@ public class BigFormatPartitionWriter extends SortedTablePartitionWriter
Version version,
ISerializer<IndexInfo> indexInfoSerializer)
{
- this(header, writer, version, indexInfoSerializer, DatabaseDescriptor.getColumnIndexCacheSize(), DatabaseDescriptor.getColumnIndexSize(DEFAULT_GRANULARITY));
+ this(header,
+ writer,
+ version,
+ indexInfoSerializer,
+ (int)version.getSSTableFormatValue(Option.COLUMN_INDEX_CACHE_SIZE),
Review Comment:
I know, thanks.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413424748
##########
src/java/org/apache/cassandra/io/sstable/format/big/BigFormatPartitionWriter.java:
##########
@@ -70,7 +70,12 @@ public class BigFormatPartitionWriter extends SortedTablePartitionWriter
Version version,
ISerializer<IndexInfo> indexInfoSerializer)
{
- this(header, writer, version, indexInfoSerializer, DatabaseDescriptor.getColumnIndexCacheSize(), DatabaseDescriptor.getColumnIndexSize(DEFAULT_GRANULARITY));
+ this(header,
+ writer,
+ version,
+ indexInfoSerializer,
+ (int)version.getSSTableFormatValue(Option.COLUMN_INDEX_CACHE_SIZE),
Review Comment:
what about add interfaces(getRowIndexCacheSize、getRowIndexGranularity and other methods) in`SSTableFormat` , and for BIG and BTI, We can implement supported options, and throw exceptions if they are not supported.
As you have said `This will likely require changes to DatabaseDescriptor.setXXX methods to make sure they update the format instances rather than the database descriptor ones.` , we may parse the value from the [option maps](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/io/sstable/format/AbstractSSTableFormat.java#L27), so I think we should add some method called `DatabaseDescriptor.setFormatOption(formatName, optionName, optionValue)` so that we can modify the options map in different instance
Besides, I still want to consult. I want to put all common options in SSTableFormat.java as [Components](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/io/sstable/format/SSTableFormat.java#L155) has done. BIG and BTI can has their own options .
##########
src/java/org/apache/cassandra/io/sstable/format/big/BigFormatPartitionWriter.java:
##########
@@ -70,7 +70,12 @@ public class BigFormatPartitionWriter extends SortedTablePartitionWriter
Version version,
ISerializer<IndexInfo> indexInfoSerializer)
{
- this(header, writer, version, indexInfoSerializer, DatabaseDescriptor.getColumnIndexCacheSize(), DatabaseDescriptor.getColumnIndexSize(DEFAULT_GRANULARITY));
+ this(header,
+ writer,
+ version,
+ indexInfoSerializer,
+ (int)version.getSSTableFormatValue(Option.COLUMN_INDEX_CACHE_SIZE),
Review Comment:
what about add interfaces(getRowIndexCacheSize、getRowIndexGranularity and other methods) in`SSTableFormat` , and for BIG and BTI, We can implement supported options, and throw exceptions if they are not supported.
As you have said `This will likely require changes to DatabaseDescriptor.setXXX methods to make sure they update the format instances rather than the database descriptor ones.` , we may parse the value from the [option maps](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/io/sstable/format/AbstractSSTableFormat.java#L27), so I think we should add some method called `DatabaseDescriptor.setFormatOption(formatName, optionName, optionValue)` so that we can modify the options map in different instance
Besides, I still want to consult. I want to put all common options in SSTableFormat.java as [Components](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/io/sstable/format/SSTableFormat.java#L155) has done. BIG and BTI can has their own options .
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413671489
##########
src/java/org/apache/cassandra/io/sstable/format/big/BigFormatPartitionWriter.java:
##########
@@ -70,7 +70,12 @@ public class BigFormatPartitionWriter extends SortedTablePartitionWriter
Version version,
ISerializer<IndexInfo> indexInfoSerializer)
{
- this(header, writer, version, indexInfoSerializer, DatabaseDescriptor.getColumnIndexCacheSize(), DatabaseDescriptor.getColumnIndexSize(DEFAULT_GRANULARITY));
+ this(header,
+ writer,
+ version,
+ indexInfoSerializer,
+ (int)version.getSSTableFormatValue(Option.COLUMN_INDEX_CACHE_SIZE),
Review Comment:
The problem with a solution like this is that it assumes we know all the options that a format may require. This cannot work if we allow formats to be plugged into Cassandra (e.g. by supplying an additional JAR with their implementation).
The format itself should deal with its options. And the rest of the code can't make options we don't know about second-hand citizens, i.e. we must treat the options we know about in the same way that we treat the options we don't know about.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1537854438
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
We do not want to support per-table configuration at the moment. This means that we don't need to support format variations (e.g. bti-fast) either.
However, we should support formats that people can plug into the database by linking with a JAR that implements their custom format. We do not know what options such formats may use.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413698000
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
thanks for your reply. In fact, what I want to focus more on is the matching issue of this format. Because if we have many configurable ones such as big-fast1/big-small1, they both support IndexSummaryConfiguration, so what should we do to make the action of match , which one should be picked up ?
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1535026413
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
Hi @blambov， long time no see, and I am back and continue working on this again. After previous communication with you above , I already have some clear directions for modifying this patch .
But I have a new question about table level configuration of [CASSANDRA-18534](https://issues.apache.org/jira/browse/CASSANDRA-18534) .
the discussion in [CASSANDRA-18441](https://issues.apache.org/jira/browse/CASSANDRA-18441) has described that we will not support per-table configuration at this point , right ? so , that means we may only have two kinds of sstable format: big or bti. And there is not going to support the feature that contains more than two kinds of sstable format configuration [here](https://github.com/apache/cassandra/blob/trunk/conf/cassandra.yaml#L1114), as I have said may be big/bti/big-small/bti-small and so on ? Am I right ?
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1395137066
##########
src/java/org/apache/cassandra/index/sai/plan/Expression.java:
##########
@@ -168,7 +168,7 @@ public Expression add(Operator op, ByteBuffer value)
break;
}
- assert operator != null;
+ assert operator != null || context.isNotIndexed() : "Cannot use '" + op + "' operator with indexed " + context.getColumnName() + " column";;
Review Comment:
Not, I remembered to remove it ~~~
Seems I should do agagin
##########
src/java/org/apache/cassandra/index/sai/plan/Expression.java:
##########
@@ -168,7 +168,7 @@ public Expression add(Operator op, ByteBuffer value)
break;
}
- assert operator != null;
+ assert operator != null || context.isNotIndexed() : "Cannot use '" + op + "' operator with indexed " + context.getColumnName() + " column";;
Review Comment:
Not, I remembered to remove it ~~~
Seems I should do again
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413354428
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
` if there are multiple, we should probably check that the options they provide match`
I think we can only get all the sstable formats provided from DatabaseDescriptor.getSSTableFormats() to fetch all the formats , there may exists some format that support IndexSummaryConfiguration but, I am not clear with `match` mean? And for me, `IndexSummaryManager` seems can not be table level, So I used `DatabaseDescriptor.getSelectedSSTableFormat()`.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413664261
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
The question is not about table level, but rather about data that already exists and is in other sstable formats (if you imagine a node with say 1 TiB of data that was written before the new format was available, and where we don't want to forcefully upgrade).
In other words, in a mixed-format environment, especially while we are switching from BIG to BTI. In this latter case the selected format (BTI) will not need index summaries and won't implement `IndexSummaryConfiguration` but we still need an `IndexSummaryManager` so that the old SSTables (in BIG format) can be dealt with properly.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1413975798
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
I don't want to overthink it, especially as there is only one format that uses these at the moment -- if there are multiple and they don't match, we should raise an exception during the validation of the yaml.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "blambov (via GitHub)" <gi...@apache.org>.
blambov commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1386607843
##########
conf/cassandra.yaml:
##########
@@ -1038,11 +1045,11 @@ snapshot_links_per_second: 0
# large rows, or a very large number of rows per partition are
# present, it is recommended to increase the index granularity
# or switch to the BTI SSTable format.
-#
+# The original name of this configuration is colum_index_size.
# Leave undefined to use a default suitable for the SSTable format
# in use (64 KiB for BIG, 16KiB for BTI).
# Min unit: KiB
-# column_index_size: 4KiB
+# row_index_granularity: 4KiB
Review Comment:
The legacy option's name cannot change, but we should deprecate it (as well as any other options that are being moved by this ticket).
Note that we also need to move the description of the option.
##########
src/java/org/apache/cassandra/io/sstable/format/AbstractSSTableFormat.java:
##########
@@ -32,6 +32,33 @@ protected AbstractSSTableFormat(String name, Map<String, String> options)
this.options = options;
}
+ public enum Option
Review Comment:
These should be defined by the individual format, because we don't know what options a format may require.
BTI, for example, only supports two of these.
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableFormat.java:
##########
@@ -45,10 +47,13 @@
public interface SSTableFormat<R extends SSTableReader, W extends SSTableWriter>
{
String name();
+ Map<String, String> options();
Version getLatestVersion();
Version getVersion(String version);
+ Object getSSTableFormatValue(Option option);
Review Comment:
This is overly restrictive. A new format (plugged via an external jar, for example) will use options which are not in the enumeration.
The general format machinery should deal only with strings. It is up only to the individual format to interpret and validate them. See e.g. how compaction and memtables are configured.
##########
src/java/org/apache/cassandra/io/sstable/format/big/BigFormatPartitionWriter.java:
##########
@@ -70,7 +70,12 @@ public class BigFormatPartitionWriter extends SortedTablePartitionWriter
Version version,
ISerializer<IndexInfo> indexInfoSerializer)
{
- this(header, writer, version, indexInfoSerializer, DatabaseDescriptor.getColumnIndexCacheSize(), DatabaseDescriptor.getColumnIndexSize(DEFAULT_GRANULARITY));
+ this(header,
+ writer,
+ version,
+ indexInfoSerializer,
+ (int)version.getSSTableFormatValue(Option.COLUMN_INDEX_CACHE_SIZE),
Review Comment:
The generic `Version` or `SSTableFormat` interfaces should not provide access to any options.
Instead, `BigFormat` should provide methods to access individual settings that the format needs, so that, for example, this code becomes
```
this(header,
writer,
version,
indexInfoSerializer,
((BigFormat) version.format).getRowIndexCacheSize(),
((BigFormat) version.format).getRowIndexGranularity());
```
where the format must parse and store the converted value (after any lookup to `DatabaseDescriptor`). This will likely require changes to `DatabaseDescriptor.setXXX` methods to make sure they update the format instances rather than the database descriptor ones.
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiFormat.java:
##########
@@ -26,6 +26,9 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
+import org.apache.cassandra.config.DataStorageSpec;
Review Comment:
Nit: these imports are in the wrong place.
##########
src/java/org/apache/cassandra/db/compaction/CompactionManager.java:
##########
@@ -1778,10 +1778,11 @@ public void close() {}
}
CompactionStrategyManager strategy = cfs.getCompactionStrategyManager();
+ SSTableFormat<?, ?> format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
Why not `cfs.getSSTableFormat()` to make it easier to switch to per-table ones?
##########
src/java/org/apache/cassandra/io/sstable/format/bti/BtiFormat.java:
##########
@@ -138,6 +144,49 @@ public Version getVersion(String version)
return new BtiVersion(this, version);
}
+ @Override
+ public Object getSSTableFormatValue(Option option)
+ {
+ String value = options.get(option.getName());
+ switch (option)
+ {
+ case ROW_INDEX_GRANULARITY:
+ if (value == null)
+ {
+ return DatabaseDescriptor.getRowIndexGranularity(DEFAULT_GRANULARITY);
+ }
+ DataStorageSpec.IntKibibytesBound resultKb = new DataStorageSpec.IntKibibytesBound(value);
+ checkValidForByteConversion(resultKb, "row_index_granularity");
+ return resultKb.toKibibytes();
+ case INDEX_SUMMARY_CAPACITY:
Review Comment:
This is not something the BTI format should or can return; it does not have such an option.
##########
src/java/org/apache/cassandra/index/sai/plan/Expression.java:
##########
@@ -168,7 +168,7 @@ public Expression add(Operator op, ByteBuffer value)
break;
}
- assert operator != null;
+ assert operator != null || context.isNotIndexed() : "Cannot use '" + op + "' operator with indexed " + context.getColumnName() + " column";;
Review Comment:
Is this change a part of the patch?
##########
src/java/org/apache/cassandra/db/compaction/CompactionManager.java:
##########
@@ -1438,7 +1438,7 @@ private void doCleanupOne(final ColumnFamilyStore cfs,
List<SSTableReader> finished;
long nowInSec = FBUtilities.nowInSeconds();
- try (SSTableRewriter writer = SSTableRewriter.construct(cfs, txn, false, sstable.maxDataAge);
+ try (SSTableRewriter writer = SSTableRewriter.construct(cfs, sstable.descriptor.getFormat(), txn, false, sstable.maxDataAge);
Review Comment:
This should use the currently selected format (we upgrade version/format on all compactions unless we are specifically doing a downgrade operation).
##########
src/java/org/apache/cassandra/io/sstable/SSTableRewriter.java:
##########
@@ -87,24 +88,24 @@ public SSTableRewriter(ILifecycleTransaction transaction, long maxAge, long pree
this.eagerWriterMetaRelease = eagerWriterMetaRelease;
}
- public static SSTableRewriter constructKeepingOriginals(ILifecycleTransaction transaction, boolean keepOriginals, long maxAge)
+ public static SSTableRewriter constructKeepingOriginals(ILifecycleTransaction transaction, SSTableFormat<?, ?> format, boolean keepOriginals, long maxAge)
{
- return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(true), keepOriginals, true);
+ return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(format, true), keepOriginals, true);
}
- public static SSTableRewriter constructWithoutEarlyOpening(ILifecycleTransaction transaction, boolean keepOriginals, long maxAge)
+ public static SSTableRewriter constructWithoutEarlyOpening(ILifecycleTransaction transaction, SSTableFormat<?, ?> format, boolean keepOriginals, long maxAge)
{
- return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(false), keepOriginals, true);
+ return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(format, false), keepOriginals, true);
}
- public static SSTableRewriter construct(ColumnFamilyStore cfs, ILifecycleTransaction transaction, boolean keepOriginals, long maxAge)
+ public static SSTableRewriter construct(ColumnFamilyStore cfs, SSTableFormat<?, ?> format, ILifecycleTransaction transaction, boolean keepOriginals, long maxAge)
{
- return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(cfs.supportsEarlyOpen()), keepOriginals, true);
+ return new SSTableRewriter(transaction, maxAge, calculateOpenInterval(format, cfs.supportsEarlyOpen()), keepOriginals, true);
}
- private static long calculateOpenInterval(boolean shouldOpenEarly)
+ private static long calculateOpenInterval(SSTableFormat<?, ?> format, boolean shouldOpenEarly)
{
- long interval = DatabaseDescriptor.getSSTablePreemptiveOpenIntervalInMiB() * (1L << 20);
+ long interval = (long) format.getSSTableFormatValue(Option.SSTABLE_PREEMPTIVE_OPEN_INTERVAL) * (1L << 20);
Review Comment:
The format will not necessarily support this. To make it accessibly in a general way, define e.g. a `PreemptiveOpenConfiguration` interface that provides this value and make the two format implement it. If the given format implements that interface, get the value from it. If not, disable early open (i.e. use -1).
##########
src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java:
##########
@@ -99,24 +106,26 @@ private IndexSummaryManager(Supplier<List<T>> indexSummariesProvider)
this.indexSummariesProvider = indexSummariesProvider;
executor = executorFactory().scheduled(false, "IndexSummaryManager", Thread.MIN_PRIORITY);
-
- long indexSummarySizeInMB = DatabaseDescriptor.getIndexSummaryCapacityInMiB();
- int interval = DatabaseDescriptor.getIndexSummaryResizeIntervalInMinutes();
+ // TODO make format get from default format
+ format = DatabaseDescriptor.getSelectedSSTableFormat();
Review Comment:
Because the format shouldn't always offer these options, we need to offer some way of recognizing an exposing them somewhat generally.
One way is to check here if the format implements an interface e.g. called `IndexSummaryConfiguration` which exposes these. BIG would implement it, and BTI would not. A further complication is that for this initialization we want to find the format that does use an index summary and get the options from there. If there's no such format, the manager should not be initialized at all (which is a good way of turning this and keycache off if not needed); if there are multiple, we should probably check that the options they provide match.
##########
src/java/org/apache/cassandra/db/compaction/Upgrader.java:
##########
@@ -92,7 +92,7 @@ public void upgrade(boolean keepOriginals)
{
outputHandler.output("Upgrading " + sstable);
long nowInSec = FBUtilities.nowInSeconds();
- try (SSTableRewriter writer = SSTableRewriter.construct(cfs, transaction, keepOriginals, CompactionTask.getMaxDataAge(transaction.originals()));
+ try (SSTableRewriter writer = SSTableRewriter.construct(cfs, sstable.descriptor.getFormat(), transaction, keepOriginals, CompactionTask.getMaxDataAge(transaction.originals()));
Review Comment:
An upgrade _must_ use the currently selected format; otherwise it is not doing what is expected of it (one of the main use cases for upgrade will be to switch to BTI from BIG).
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1389457901
##########
conf/cassandra.yaml:
##########
@@ -1038,11 +1045,11 @@ snapshot_links_per_second: 0
# large rows, or a very large number of rows per partition are
# present, it is recommended to increase the index granularity
# or switch to the BTI SSTable format.
-#
+# The original name of this configuration is colum_index_size.
# Leave undefined to use a default suitable for the SSTable format
# in use (64 KiB for BIG, 16KiB for BTI).
# Min unit: KiB
-# column_index_size: 4KiB
+# row_index_granularity: 4KiB
Review Comment:
ok
##########
src/java/org/apache/cassandra/io/sstable/format/AbstractSSTableFormat.java:
##########
@@ -32,6 +32,33 @@ protected AbstractSSTableFormat(String name, Map<String, String> options)
this.options = options;
}
+ public enum Option
Review Comment:
Yes, at first, I set it up separately in big and bti format. But I found that the two have some common options, so my initial idea is that AbstractSSTableFormat includes all, and then different formats are selectively supported based on their own capabilities. For those that are not supported, exceptions are thrown. see the [test here ](https://github.com/Maxwell-Guo/cassandra/blob/CASSANDRA-18533/test/unit/org/apache/cassandra/io/sstable/SSTableFormatTest.java#L388)
But after read your comment on ["I would keep it simple and not add a common settings entry under options. If necessary, the user can copy the value to both."](https://issues.apache.org/jira/browse/CASSANDRA-18533) ,I think it's better to change the option to individual format.
--
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] Move format-specific sstable options into the format configuration [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2879:
URL: https://github.com/apache/cassandra/pull/2879#discussion_r1389460108
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableFormat.java:
##########
@@ -45,10 +47,13 @@
public interface SSTableFormat<R extends SSTableReader, W extends SSTableWriter>
{
String name();
+ Map<String, String> options();
Version getLatestVersion();
Version getVersion(String version);
+ Object getSSTableFormatValue(Option option);
Review Comment:
I noticed this ,if I want to fix the upper comment , the input must be String , thanks for your longer term explanation
--
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