You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "millerjp (via GitHub)" <gi...@apache.org> on 2026/09/25 07:41:36 UTC
[PR] CASSANDRA-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
millerjp opened a new pull request, #5213:
URL: https://github.com/apache/cassandra/pull/5213
The datastax/cassandra fix by @blambov (datastax/cassandra@5a6f917bc3) rebased onto cassandra-5.0 branch, keeping the original author. I only changed what would not compile. This branch has no CompactionRealm, so the table partition count is read from TableMetrics in ShardManager.create, and the ShardManagerReplicaAware changes were dropped because that class does not exist on this branch.
Also added a unit test that checks tiny sstables covering a very small slice of the token range eventually get compacted, and two dtests that run six rounds of repair (full subrange and incremental) and check that the small sstables streamed by repair do not pile up. These dtests failed against the original commit on the big format which is why an additional fix was added for big.
The original commit uses estimatedKeys() < 100 to identify small sstables. That works on bti but not on big where the count comes from the index summary and can never be lower than min_index_interval (128), so a fix was needed for big. As suggested by @driftx on the ticket the count is now read from getEstimatedPartitionSize().count() which works on both formats with a fallback to estimatedKeys() if for whatever reason partition sizes are null or 0 (unlikely, but thought it best to add this just in case).
ShardManagerTest, UnifiedCompactionStrategyTest, ShardedCompactionWriterTest, ControllerTest, CompactionsCQLTest and CompactionStrategyManagerPendingRepairTest all pass. UnifiedCompactionDensitiesTest passes on the default config and the two repair dtests pass on bti and big.
Co-authored-by: @blambov
PRs for cassandra-6.0 and trunk will follow.
Fixes https://issues.apache.org/jira/browse/CASSANDRA-21615 on 5.0
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-6371700065
@michaelsembwever following up on the rollback question, I added MaxSSTablesPerShardFactorSchemaUpgradeTest, based on AutoRepairDisabledSchemaUpgradeTest. It runs two nodes on 5.0.7, the old version CI already builds a dtest jar for, upgrades one and rolls it back.
- testDefaultFactorUpgradeAndRollback: nothing set on the table. The upgraded node compacts an oversize shard with the default factor of 10 (21 single row sstables, limit 20 at T2). Rolling it back to 5.0.7 needs nothing: schema agreement holds, ALTER TABLE ... ADD works on both nodes and every row reads back, including the ones compacted on the new version.
- testMaxSSTablesPerShardFactorUpgradeAndRollback: the option set on the table through the upgraded node. The 5.0.7 node keeps schema agreement, reads, writes and flushes, and accepts ALTER TABLE ... WITH, but rejects ALTER TABLE ... ADD with "not understood by UnifiedCompactionStrategy". Setting the compaction options again without the option removes it, after which the rollback works and both nodes accept ALTER TABLE ... ADD again.
So there's no need to touch system_schema. A normal ALTER TABLE that sets the compaction options without max_sstables_per_shard_factor removes it, and the NEWS.txt upgrading note says to do that before rolling back.
To run it locally, build dtest-5.0.7.jar from the cassandra-5.0.7 tag and dtest-5.0.10.jar from this branch (ant dtest-jar), and put both in build/.
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-6333951145
Thanks @michaelsembwever, good catch. I added a test first to confirm it, then applied your fix as suggested, getNumShards(max * context.shardManager.shardSetCoverage()) (e0e76d882e).
The new tests use a real controller instead of the mocked getNumShards:
- one data directory with 1/8 ownership: before the fix it counted 8 shards for 8.8 GiB and picked nothing, after it counts 1 and picks all eight sstables
- two disks with 1/4 ownership, so the shard set is one disk: this also fails if the density is scaled to the node's share instead of the disk's
- the default sharding options, which go through the min size and growth paths of getNumShards that the other tests only reached through mocks
While on it I added two more (b976050ec0):
- a unit test for the chooseCompactionPick change with per level scaling parameters, where a level with a higher overlap but no pick used to discard a lower level's pick. It fails on the cassandra-5.0 loop.
- a jvm dtest that runs the trigger end to end on a node that owns half the ring, with the factor set through CQL: six disjoint sstables in one shard are compacted with a factor of 1 and left alone with 0.
And a NEWS upgrade note (3def0d85ae). A 5.0 node without this patch rejects ALTER TABLE ... ADD on a table that sets max_sstables_per_shard_factor, because that path validates the whole table and the option is unknown there. ALTER TABLE ... WITH is not affected. I checked it on cassandra-5.0. The note says to set the option only once every node is upgraded, and to remove it before a downgrade.
One more thing. While checking the open PRs for the flaky tests from your CI runs, I saw that Dmitry's #5269 (CASSANDRA-21769) makes the same fix as my commit e1f6d79006: it stubs the expired sstable check frequency in testDropExpiredFromBucket and testDropExpiredAndCompactNonExpired. 21769 has 5.0.x in its fix versions, so the two would conflict on 5.0. Happy to drop e1f6d79006 from this PR and leave it to 21769.
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-5833001871
1. [P1] An empty table can keep a full-range span floor.
`src/java/org/apache/cassandra/db/compaction/ShardManager.java:79` replaces a zero partition estimate, but the table metric returns -1 when there are no SSTables or memtable partitions.
`src/java/org/apache/cassandra/db/compaction/ShardManagerNoDisks.java:56` clamps that to one, so each later SSTable with fewer than 100 partitions can receive the entire local range as its minimum span. Should treat every nonpositive estimate as unavailable.
3. [P2] The partition estimate goes stale.
`src/java/org/apache/cassandra/db/compaction/ShardManagerNoDisks.java:56` fixes the span floor at construction. UCS rebuilds the manager only when the ring version changes, so flushes and compactions leave the floor tied to an earlier table size. This changes tiny SSTable density and level placement as the table grows or shrinks. Refresh the estimate when measuring span, or rebuild the manager when the estimate changes.
4. [P2] Compaction output sizing still uses the raw span.
`src/java/org/apache/cassandra/db/compaction/ShardManager.java:208` applies the new floor to individual SSTables, but calculateCombinedDensity still divides by the raw token span. Two small SSTables spanning 1e-11 of the ring can enter a low density level, then produce a combined density near 200 TB for output shard sizing. Apply a consistent floor to the combined estimate.
5. [P2] The distributed regression can pass without testing the defect.
`test/distributed/org/apache/cassandra/distributed/test/UnifiedCompactionDensitiesTest.java:210` asserts only that the final count of files under 8 KiB is below six. Zero such files in every round satisfies the assertion. The test should first prove repair produced a targeted tiny SSTable, then prove background compaction consumed one.
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-6315621043
Potential bug:
We need to normalize density before calculating fallback shards.
`UnifiedCompactionStrategy.java:714` passes max as whole-ring density, but `getNumShards` requires density scaled to local ownership or one disk.
For example, with ownership 1/8, maximum density 8 GiB, target size 1 GiB, base count 1, and growth 0, this produces eight counting shards instead of one. Eight disjoint SSTables, one per counting shard, escape a threshold of two although the intended shard exceeds it.
Fix:
`controller.getNumShards(max * context.shardManager.shardSetCoverage())`
The existing tests mock `getNumShards(anyDouble())` to a constant, hiding the error. Add a regression using the real controller with partial ownership. (@blambov , DataStax’s current fork contains the same unscaled call.)
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-6368214627
CASSANDRA-21769 has landed on cassandra-5.0, so I rebased and dropped my duplicate commit. The PR is mergeable 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] CASSANDRA-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-5837055844
@michaelsembwever thanks for the review. All four points fixed, one commit each:
1. Nonpositive estimates are now treated as unavailable. Worth noting the -1 comes back for any empty table so the same check is wrong in the original datastax commit too.
2. (your point 4) calculateCombinedDensity now applies the same floor as rangeSpanned, with the same partition count gate, plus a unit test covering both sides of the gate.
3. The shard manager is now rebuilt when the partition count estimate changes, not only on ring changes, plus a unit test for the staleness check. One trade off to be aware of: the estimate gauge walks the sstable stats, and it is now read on every maybeUpdateShardManager call instead of only when the manager is rebuilt. Under write load the estimate changes often, so the manager also rebuilds much more often than before. If that cost is a concern on tables with very high sstable counts I can throttle the check or only rebuild when the estimate moves by some factor. Let me know which way you want it.
4. (your point 5) the dtests now record the tiny sstable count right after each repair, before compaction gets a chance to run, and fail if no round ever observed one.
Same commits will go to #5214 and #5215 once you are happy with these.
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#discussion_r4104094566
##########
src/java/org/apache/cassandra/db/compaction/ShardManager.java:
##########
@@ -152,6 +153,22 @@ default double rangeSpanned(SSTableReader rdr)
return span > perPartitionMinimum ? span : perPartitionMinimum; // The latter will be chosen if span is NaN too.
}
+ /**
+ * The number of partitions in the given sstable, read from its stats metadata, which stores the exact
+ * count for every sstable format. estimatedKeys() is not used here because the big format derives it from
+ * the index summary and cannot return fewer than min_index_interval (128 by default) keys, which is above
+ * PER_PARTITION_SPAN_THRESHOLD. See CASSANDRA-21615.
Review Comment:
```suggestion
* The number of partitions in the given sstable, read from its stats metadata, which stores the exact
* count for every sstable format.
```
is probably enough. we don't need "checkpoint" explanations. and the ticket can be found via git blame.
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on code in PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#discussion_r4104131848
##########
src/java/org/apache/cassandra/db/compaction/ShardManager.java:
##########
@@ -152,6 +153,22 @@ default double rangeSpanned(SSTableReader rdr)
return span > perPartitionMinimum ? span : perPartitionMinimum; // The latter will be chosen if span is NaN too.
}
+ /**
+ * The number of partitions in the given sstable, read from its stats metadata, which stores the exact
+ * count for every sstable format. estimatedKeys() is not used here because the big format derives it from
+ * the index summary and cannot return fewer than min_index_interval (128 by default) keys, which is above
+ * PER_PARTITION_SPAN_THRESHOLD. See CASSANDRA-21615.
Review Comment:
Will backport this to 6.0 and trunk PRs also
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-5980814347
@millerjp , please remove the commit `Merge branch 'cassandra-5.0' into millerjp/CASSANDRA-21615/cassandra-5.0`
(this can't be in the branch)
--
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-21615 UCS never compacts tiny high-density SSTables streamed by repair (5.0 branch) [cassandra]
Posted by "millerjp (via GitHub)" <gi...@apache.org>.
millerjp commented on PR #5213:
URL: https://github.com/apache/cassandra/pull/5213#issuecomment-6283631292
@michaelsembwever @blambov the branch is updated as asked. The merge commit is gone, the branch is rebased on cassandra-5.0 and the earlier commits are unchanged in content. The three datastax commits are in as one commit each, original authors kept and the source hash in each message:
- datastax/cassandra@c93158a3 cached partition count estimate (4f418a5e55)
- datastax/cassandra@0a08dccc five minute cache period (6abb14950e)
- datastax/cassandra@eade48b5 max_sstables_per_shard_factor (d4cb895abf)
In short the patch now does four things. It floors how small an sstable's ring slice can be, so a two partition sstable is measured as two partitions' worth of ring and gets a density in line with the real data (the original datastax fix plus the big format partition count from stats metadata). It gives compaction a cheap partition count, cached per sstable set with a five minute copy for compaction that leaves memtables out, and rebuilds the shard manager when the estimate changes. It adds a backstop for the cases overlap never catches: when a level has no overlap group to compact and a shard holds more sstables than max_sstables_per_shard_factor times the fanout, that shard is compacted outright. And it makes that factor a proper table option, validated through CQL, documented, in cqlsh completion and in NEWS.txt.
On the option itself, since it is the user facing part. max_sstables_per_shard_factor is a UCS compaction option set per table in the compaction map, the same way datastax defines it, for example `ALTER TABLE ks.t WITH compaction = {'class': 'UnifiedCompactionStrategy', 'max_sstables_per_shard_factor': 20}`. A table that does not set it gets the node default, which is 10 unless `-Dunified_compaction.max_sstables_per_shard_factor` changes it, like the other UCS options. At 10 and the default T4 fanout a shard is compacted once it holds more than 40 sstables. 0 turns it off for that table. CHANGES.txt is reworded to name both mechanisms and NEWS.txt has an entry for the option.
This branch has no CompactionRealm or CompactionAggregate, so a few things had to be adapted. The count goes through ShardManager.estimatedPartitionCountInSSTables(ColumnFamilyStore), which reads the new cached gauge. The oversize shard trigger sits in the one pick per call flow: Level.getCompactionPick returns the oversize pick when the level has no overlap bucket, and chooseCompactionPick skips levels without a pick so that a pick with an overlap below the threshold is not thrown away for an idle level. With the usual uniform thresholds this selects exactly as before. One side effect worth knowing: with per level scaling parameters where a lower level has a higher threshold than a higher one, stock could discard the higher level's pick in favour of nothing, and that no longer happens. ShardManager gets a splitSSTablesInShards default method (the datastax assignSSTablesInShards walk). ShardManagerTrivial reports the minimum token as its shard end where datastax returns null, so t
he walk treats that as unbounded, otherwise it never terminates on a partitioner without a splitter. The option default also comes from a system property as in datastax, through a new CassandraRelevantProperties entry, and there is no cassandra.yaml entry.
Beyond the ports, in separate commits so they are easy to drop if you disagree:
- more tests around the trigger, the shard split, the span floor on the disk aware manager and the estimate driven shard manager rebuild, plus a dtest that the shard manager is rebuilt when the estimate changes (f2c87c084d)
- CQL validation of the option on CREATE TABLE and ALTER TABLE with the exact error text, and the stored value read back from system_schema (5c0904c82d)
- the option documented in the UCS options table on the compaction page and in the CQL compaction subproperties reference, and added to cqlsh tab completion with its completion test (0dee89867e). I followed the neighbouring entries, plain default and no mention of the system property, since no existing UCS option documents one
- a two line fix for testDropExpiredFromBucket and testDropExpiredAndCompactNonExpired (e1f6d79006), which fail intermittently on cassandra-5.0 without this patch because the expiry check only runs once per wall clock millisecond. Happy to move that one to its own ticket if you prefer
Two places where I deliberately went a different way from datastax, please shout if you want them the datastax way:
- 0 disables the trigger (f9f158be16). There was no documented way to turn it off short of a very large value, and the project turns numeric knobs off with 0 (min_sstable_size, max_sstables_to_compact, the yaml throttles). Validation accepts 0 or a number of at least 1, the trigger returns no pick at 0 (it would otherwise fire on every level), and the value from the system property goes through the same check. Datastax validates >= 1 only and uses the system property value unvalidated.
- NaN is rejected. The datastax check is `value < 1`, which is false for NaN, so NaN passed validation and silently disabled the trigger. The datastax copy has the same hole.
A few things found on the way that you may want to pass back to datastax. The five minute cache pins a brand new table's estimate at the initial 1 << 16 for five minutes, because the first read happens at the first flush before any sstable exists and 0 is cached. The repair dtests on this branch failed on both formats because of it, so the dtest shortens the period on each instance by reflection (the same lever TableMetricsTest uses) rather than changing the source. If you would rather the empty estimate is not cached, that is a small change in ShardManager.estimatedPartitionCountInSSTables. The oversize pick is not capped by max_sstables_to_compact, same as datastax, say if you want a cap on Apache. And the oversize pick counts as one pending task in getEstimatedRemainingTasks so compactionstats shows work for such a level.
--
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