Cleanup compaction / rewrite sstable block behind regular compaction for each sstable

[bhalevy]

Opening this issue based on my analysis in #10060 (comment)

With 40a6049, rewrite_sstables takes the write lock for each sstable.
Since it will block behind regular compaction, the whole cleanup task might take a very long time.

The purpose of this change was to serialize cleanup or upgrade_sstables with other "maintenance" compaction tasks.

This was achieved in a better way in 6737c88, and after this change rewrite_sstables can safely take the read lock rather the write lock, and let regular compaction run in parallel without interruption.

That said, I don't understand why rewrite_sstables acquires the _maintenance_ops_sem per sstable and not once for the whole task.

[bhalevy]

This is hard to reproduce in dtest, since we need large sstables and probably heavy write load to trigger long compactions in parallel to cleanup (or upgrade sstables for that matter).

I was able to reproduce cleanup slowdown in simulation by injecting 1 second sleep to each compaction job,
taking ~37 seconds to cleanup 50 sstables.
When removing the write lock cleanup time went down to little less than a second (all in dev mode).

[bhalevy]

Also, since _maintenance_ops_sem is global to the compaction_manager, we're compacting one sstable at a time per shard across the whole database.

I have a patch that moves the serialization of maintenance ops to the caller layer (api / distributed_loader), and makes it per-keyspace, so we can run one (sharded) major or offstrategy compaction per keyspace at a time, vs. one (sharded) cleanup / upgrade_sstables / scrub per table in the keyspace since the latter, that use rewrite_sstables, are compacting only a single sstable at a time.

[bhalevy]

Also refs #10097 that is focused on adding the output sstables in batches, to reduce write amplification and increase compaction efficiency of the regular compaction that runs in parallel in cleanup/upgrade sstables.

[raphaelsc]

Also, since _maintenance_ops_sem is global to the compaction_manager, we're compacting one sstable at a time per shard across the whole database.

I have a patch that moves the serialization of maintenance ops to the caller layer (api / distributed_loader), and makes it per-keyspace, so we can run one (sharded) major or offstrategy compaction per keyspace at a time, vs. one (sharded) cleanup / upgrade_sstables / scrub per table in the keyspace since the latter, that use rewrite_sstables, are compacting only a single sstable at a time.

I think this could be a regression. Maintenance operations cannot be made faster by running more of them. One cleanup per shard is enough to saturate its available bandwidth, as compaction takes advantage of read ahead and write behind for disk utilization.

[raphaelsc]

Opening this issue based on my analysis in #10060 (comment)

With 40a6049, rewrite_sstables takes the write lock for each sstable. Since it will block behind regular compaction, the whole cleanup task might take a very long time.

The purpose of this change was to serialize cleanup or upgrade_sstables with other "maintenance" compaction tasks.

This was achieved in a better way in 6737c88, and after this change rewrite_sstables can safely take the read lock rather the write lock, and let regular compaction run in parallel without interruption.

Thanks for working on this Benny! It definitely makes sense making rewrite_sstables() acquire the read lock instead.

That said, I don't understand why rewrite_sstables acquires the _maintenance_ops_sem per sstable and not once for the whole task.

Yes, that's definitely asking for more trouble as we could have cleanup of different tables running intermixed. It's better to allow one at a time.

[bhalevy]

Also, since _maintenance_ops_sem is global to the compaction_manager, we're compacting one sstable at a time per shard across the whole database.
I have a patch that moves the serialization of maintenance ops to the caller layer (api / distributed_loader), and makes it per-keyspace, so we can run one (sharded) major or offstrategy compaction per keyspace at a time, vs. one (sharded) cleanup / upgrade_sstables / scrub per table in the keyspace since the latter, that use rewrite_sstables, are compacting only a single sstable at a time.

I think this could be a regression. Maintenance operations cannot be made faster by running more of them. One cleanup per shard is enough to saturate its available bandwidth, as compaction takes advantage of read ahead and write behind for disk utilization.

Why do we run parallel compaction jobs on multiple buckets at a time then?
I don't think the upper level logic should second guess resource scheduling, but rather it should do a very rough throttling and let the cpu / mem / disk schedulers do their job based on the perceived load.

If e.g. scrub is running on one keyspace or table, why not allow major compaction to run on another keyspace?
We allow regular compaction running there in parallel, don't we?

Or if we run resharding for nodetool refresh, that works on a specific table's uploads dir, why does it need to take a shard-global lock and prevent e.g. major compaction on an unrelated keyspace or table?

[bhalevy]

we could have cleanup of different tables running intermixed. It's better to allow one at a time.

I'm not sure this is a problem.
We allow regular compaction on all tables in parallel and that's just fine.
If we need to throttle then number of compaction jobs running in parallel to cap the overall resources compaction is consuming, we should have a concurrency semaphore similar to the one we use for queries.
I don't think we should use a shard-global high level semaphore for that.
The latter should be used at the api layer to serialize high-level maintenance operations where needed.

[raphaelsc]

Also, since _maintenance_ops_sem is global to the compaction_manager, we're compacting one sstable at a time per shard across the whole database.
I have a patch that moves the serialization of maintenance ops to the caller layer (api / distributed_loader), and makes it per-keyspace, so we can run one (sharded) major or offstrategy compaction per keyspace at a time, vs. one (sharded) cleanup / upgrade_sstables / scrub per table in the keyspace since the latter, that use rewrite_sstables, are compacting only a single sstable at a time.

I think this could be a regression. Maintenance operations cannot be made faster by running more of them. One cleanup per shard is enough to saturate its available bandwidth, as compaction takes advantage of read ahead and write behind for disk utilization.

Why do we run parallel compaction jobs on multiple buckets at a time then? I don't think the upper level logic should second guess resource scheduling, but rather it should do a very rough throttling and let the cpu / mem / disk schedulers do their job based on the perceived load.

If e.g. scrub is running on one keyspace or table, why not allow major compaction to run on another keyspace? We allow regular compaction running there in parallel, don't we?

Or if we run resharding for nodetool refresh, that works on a specific table's uploads dir, why does it need to take a shard-global lock and prevent e.g. major compaction on an unrelated keyspace or table?

We do allow parallel compaction jobs on multiple buckets for reducing read amplification, never for increasing compaction efficiency. In the past, manager was very keen at running more jobs in parallel, but that was a terrible mistake, as efficiency is a function of informed decisions, not parallelism.

For example, if we serialized compaction on a table, and we're compacting 64G of data at 1GB/s, and if we're flushing new files at the same rate, by the time compaction completes, we'd have generated 64 files in level 0.

We can of course allow maintenance operations to go in parallel, but we only have to keep in mind we're not doing so for increasing efficiency.

[raphaelsc]

we could have cleanup of different tables running intermixed. It's better to allow one at a time.

I'm not sure this is a problem. We allow regular compaction on all tables in parallel and that's just fine. If we need to throttle then number of compaction jobs running in parallel to cap the overall resources compaction is consuming, we should have a concurrency semaphore similar to the one we use for queries. I don't think we should use a shard-global high level semaphore for that. The latter should be used at the api layer to serialize high-level maintenance operations where needed.

I don't see any good reason to run more than one cleanup for multiple tables at a time.

But I can see very good reasons for running cleanup for a single table a a time, which is resiliency, we can start with smallest table to increase chances of success later on. Implementation of #10097 becomes more efficient as the cleanup for a table T will not be fighting for resources with cleanup for another table, and so the proposed batching can be done faster.

[bhalevy]

Should be backported to 4.6 based on #10060 (comment)

[denesb]

@bhalevy please prepare a PR with the backport to 4.6, it doesn't apply cleanly (it applies cleanly to 5.0).

[bhalevy]

@bhalevy please prepare a PR with the backport to 4.6, it doesn't apply cleanly (it applies cleanly to 5.0).

@denesb patch sent to scylladb-dev

[denesb]

Backported to all vulnerable branches (4.6, 5.0), removing "Backport candidate" label.