api: add compaction offstrategy option #9095

bhalevy · 2021-07-26T11:41:03Z

Trigger offstrategy compaction via the REST API with a offstrategy option.

This is useful for triggering offstrategy compaction
post repair, after repairing all token ranges.

Otherwise, offstrategy compaction will only be
auto-triggered after a 5 minutes idle timeout.

Like major compaction, if offstrategy compaction
is requried the api call waits on its future
and returns its status.

Test: unit(dev)
DTest: repair_additional_test:RepairAdditionalTest.{repair_option_pr_{multi_dc,dc_host}_test,repair_option_cf_test}

Signed-off-by: Benny Halevy [email protected]

api/storage_service.cc

compaction/compaction_manager.cc

api/storage_service.cc

bhalevy · 2021-08-05T10:35:28Z

In v3:

keep void compaction_manager::submit_offstrategy()
expose future<> compaction_manager::do_submit_offstrategy()
- self-contained, checked if offstrategy compaction needs triggering, otherwise returns a ready future.
- use that from api call via future<> table::submit_offstrategy_compaction()

raphaelsc

LGTM

bhalevy · 2021-08-05T11:46:12Z

@amnonh if you have no objection, @scylladb/scylla-maint let's merge this change, since the following chain of PRs depend on it: scylladb/scylla-jmx#172 -> scylladb/scylla-tools-java#269 -> scylladb/scylla-dtest#2237.

amnonh · 2021-08-05T14:04:45Z

LGTM

nyh

Looks mostly good, but I left a couple of comments for your consideration.

database.hh

table.cc

bhalevy · 2021-08-08T07:56:34Z

In v4:

compaction_manager::do_submit_offstrategy -> perform_offstrategy
table::submit_offstrategy_compaction -> perform_offstrategy_compaction
do not checkout if _off_strategy_trigger is armed, always call table::perform_offstrategy_compaction.
table: offstrategy compaction: add minimal documentation to api methods

nyh

Looks mostly good to me, but I left some small comments/requests behind.

nyh · 2021-08-08T16:10:34Z

compaction/compaction_manager.cc

@@ -623,7 +623,7 @@ void compaction_manager::submit(column_family* cf) {
    });
 }

-void compaction_manager::submit_offstrategy(column_family* cf) {
+future<> compaction_manager::perform_offstrategy(column_family* cf) {


The commit message still refers to the old names.

nyh · 2021-08-08T16:15:39Z

api/api-doc/storage_service.json

+                  },
+                  {
+                     "name":"offstrategy",
+                     "description":"When set to true, trigger offstrategy compaction (and return immediately)",


If we don't provide a better explanation of what this "offstrategy compaction" means, we need to create documentation about it - Google didn't find any. As a user I would have no idea what exactly will happen when I "trigger offstrategy compaction" or why or when I would want to do it.

I opened #9162 as a follow-up.

nyh · 2021-08-08T16:27:02Z

database.hh

    future<> perform_offstrategy_compaction();
+    // Run offstrategy compaction.
+    // Typically called only internally by the compaction manager or unit tests.


This explanation doesn't explain to me how or why run_offstrategy_compaction() and perform_offstrategy_compaction() differ - they have the same signature.

Maybe we should consider moving run_offstrategy_compaction to be private and befriend its callers.

That said, I talked with Raphael already about defining a formal abstraction of the table/compaction api so that compaction-manager and compaction would be provided with an object through which they would call back into the table (or test) code, rather than calling them directly on the table. This way compaction will operate on an abstract container of sstables that could be a table object, but it could also be a TWCS bucket, for example, or just a bunch of sstables in a temporary directory for testing.

Return a future from perform_offstrategy, resolved when the offstrategy compaction completes so that callers can wait on it. submit_offstrategy still submits the offstrategy compaction in the background. Signed-off-by: Benny Halevy <[email protected]>

So it would be easier to relate the messages to the table for which it was submitted. Signed-off-by: Benny Halevy <[email protected]>

Perform offstrategy compaction via the REST API with a `offstrategy` option. This is useful for performing offstrategy compaction post repair, after repairing all token ranges. Otherwise, offstrategy compaction will only be auto-triggered after a 5 minutes idle timeout. Like major compaction, the api call returns the offstrategy compaction task future, so it's waited on. Signed-off-by: Benny Halevy <[email protected]>

And rename it to do_run_offstrategy_compaction in the process. It is meant to be called back from the compaction manager. "Users" of the table API should now call `perform_offstrategy_compaction`. Signed-off-by: Benny Halevy <[email protected]>

It is hard to tell which of {trigger,perform,run}_offstrategy_compaction does what. Signed-off-by: Benny Halevy <[email protected]>

bhalevy · 2021-08-09T08:05:31Z

In v5:

fixed commitlog message re: compaction_manager::perform_offstrategy
made table::run_offstrategy_compaction private
- and renamed to do_run_offstrategy_compaction

avikivity · 2021-08-09T08:06:49Z

What's the difference between offstrategy and major compaction?

If the difference between the current table shape and the strategy goal is large, then offstrategy will perform something similar to major compaction, no?

And if it's small, then the compaction strategy will work to reduce it.

I'm sure there is some difference, but does it merit a new API?

bhalevy · 2021-08-09T08:13:09Z

What's the difference between offstrategy and major compaction?

If the difference between the current table shape and the strategy goal is large, then offstrategy will perform something similar to major compaction, no?

And if it's small, then the compaction strategy will work to reduce it.

I'm sure there is some difference, but does it merit a new API?

Major compaction's goal is different than offstrategy compaction.
The former's goal, e.g. with STCS, is to compact all data into a single sstable,
while the latter's goal is to bring the sstables closer to the steady-state compaction-strategy invariant,
i.e. a state equivalent to after regular compaction (of all buckets).

nyh · 2021-08-09T09:58:16Z

Major compaction's goal is different than offstrategy compaction.
The former's goal, e.g. with STCS, is to compact all data into a single sstable,
while the latter's goal is to bring the sstables closer to the steady-state compaction-strategy invariant,
i.e. a state equivalent to after regular compaction (of all buckets).

So one might say that "off-strategy" compaction is a confusing name - it's doesn't do something "off-strategy" but quite the opposite - it tries to return the current off-strategy situation to be more like the strategy wants. Maybe it should be called "back-to-strategy compaction" :-) In any case, we clearly need documentation for this feature - the one-line string in the REST API explains nothing - but I saw you opened an issue for that.

raphaelsc · 2021-08-09T10:26:50Z

On Mon, Aug 9, 2021, 6:58 AM nyh ***@***.***> wrote: Major compaction's goal is different than offstrategy compaction. The former's goal, e.g. with STCS, is to compact all data into a single sstable, while the latter's goal is to bring the sstables closer to the steady-state compaction-strategy invariant, i.e. a state equivalent to after regular compaction (of all buckets). So one might say that "off-strategy" compaction is a confusing name - it's doesn't do something "off-strategy" but quite the opposite - it tries to return the current off-strategy situation to be more like the strategy wants. Maybe it should be called "back-to-strategy compaction" :-) In any case, we clearly need documentation for this feature - the one-line string in the REST API explains nothing - but I saw you opened an issue for that.

Actually it's called offstrategy because it's a compaction performed in a maintainance sstable set where sstables in there were created by maintenance operations (they aren't even considered by regular compaction therefore offstrategy). The offstrategy compaction is not even driven by compaction shares, maintenance shares is used instead. The offstrategy goal is to prepare the maintenance sstables for integration into the main set. —

…

You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9095 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKYA4Z3TJLHMOVZP6HYU4TT36REHANCNFSM5A75Q6XA> .

nyh · 2021-08-09T10:33:11Z

Actually it's called offstrategy because it's a compaction performed in a maintainance sstable set where sstables in there were created by maintenance operations (they aren't even considered by regular compaction therefore offstrategy).

Oh! This is quite different from what @bhalevy wrote above - are you saying that "off strategy compaction" doesn't even touch the "normal" sstables which major compaction works on - rather, it only works on some new sstables recently created by maintenance operations (which operations)? So if you didn't recently do any maintenance operations, it will not do anything at all?

raphaelsc · 2021-08-09T10:38:37Z

On Mon, Aug 9, 2021, 7:33 AM nyh ***@***.***> wrote: Actually it's called offstrategy because it's a compaction performed in a maintainance sstable set where sstables in there were created by maintenance operations (they aren't even considered by regular compaction therefore offstrategy). Oh! This is quite different from what @bhalevy <https://github.com/bhalevy> wrote above - are you saying that "off strategy compaction" doesn't even touch the "normal" sstables which major compaction works on - rather, it only works on some new sstables recently created by maintenance operations (which operations)? So if you didn't recently do any maintenance operations, it will not do anything at all?

Correct —

…

You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9095 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKYA42CGITMCR6JNHTSZADT36VHFANCNFSM5A75Q6XA> .

bhalevy · 2021-08-09T11:34:21Z

Actually it's called offstrategy because it's a compaction performed in a maintainance sstable set where sstables in there were created by maintenance operations (they aren't even considered by regular compaction therefore offstrategy).

Oh! This is quite different from what @bhalevy wrote above - are you saying that "off strategy compaction" doesn't even touch the "normal" sstables which major compaction works on - rather, it only works on some new sstables recently created by maintenance operations (which operations)? So if you didn't recently do any maintenance operations, it will not do anything at all?

Yeah, this is better put this way.

bhalevy · 2021-08-12T06:36:15Z

@nyh so what's the verdict? Are you ok with merging this change?

bhalevy · 2021-08-12T06:36:57Z

@avikivity please ack. Are you ok with this change?

avikivity · 2021-08-12T09:21:13Z

Actually it's called offstrategy because it's a compaction performed in a maintainance sstable set where sstables in there were created by maintenance operations (they aren't even considered by regular compaction therefore offstrategy).

Oh! This is quite different from what @bhalevy wrote above - are you saying that "off strategy compaction" doesn't even touch the "normal" sstables which major compaction works on - rather, it only works on some new sstables recently created by maintenance operations (which operations)? So if you didn't recently do any maintenance operations, it will not do anything at all?

Yeah, this is better put this way.

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

bhalevy · 2021-08-12T11:45:20Z

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

It's not a requirement, just an optimization.
With regular repair, we don't know exactly when the meta-operation is finished since it is managed externally, e.g. by scylla-manager. Therefore, the decision was that we arm a 5 minutes timer that will submit the offstrategy compaction, if scylla is idle and no new repair operation starts.

This means that in the normal case we have a 5 minutes delay after repair finishes and until offstrategy compaction starts.
5 minutes is typically shorter than the time to run repair, but still it's a pretty long window, so the idea was to use this api to perform the offstrategy compaction immediately, when the external scheduler (e.g. scylla-manager, or a test) is done with repair and offstrategy compaction is good to go.

nyh · 2021-08-12T13:02:35Z

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

It's not a requirement, just an optimization.

This will need to be documented well - if it's at all useful... The first description I read in this patch made it sound like "off-strategy compaction" is a legitimate alternative to "major compaction", that just behaves a bit differently in a way I didn't understand. Now it has become clear that "off-strategy compaction" only useful after some unspecified "management operations" (which specific operations are those? repair? anything else?), but even that happens even without this command, so what this command apparently does is to expedite this compaction which will happen in 5 minutes anyway. None of this can be understood by the description of that command.

With regular repair, we don't know exactly when the meta-operation is finished since it is managed externally, e.g. by scylla-manager. Therefore, the decision was that we arm a 5 minutes timer that will submit the offstrategy compaction, if scylla is idle and no new repair operation starts.

Maybe the default shouldn't be 5 minutes - that users might find a long boring wait, and look for a command to expedite this. What if, instead, it was a 1 minute delay? 1 minute is more than enough for scylla-manager to start another repair if it wants, but it's not long enough for users to go looking for a command to cancel that delay.

P.S. it's not that I think that there's anything "wrong" with this new API option, it's just that wouldn't it be better if it didn't have to exist, and users wouldn't even need to learn what it means?

avikivity · 2021-08-12T13:05:59Z

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

It's not a requirement, just an optimization.
With regular repair, we don't know exactly when the meta-operation is finished since it is managed externally, e.g. by scylla-manager. Therefore, the decision was that we arm a 5 minutes timer that will submit the offstrategy compaction, if scylla is idle and no new repair operation starts.

This means that in the normal case we have a 5 minutes delay after repair finishes and until offstrategy compaction starts.
5 minutes is typically shorter than the time to run repair, but still it's a pretty long window, so the idea was to use this api to perform the offstrategy compaction immediately, when the external scheduler (e.g. scylla-manager, or a test) is done with repair and offstrategy compaction is good to go.

So, this API call is to save five minutes?

bhalevy · 2021-08-15T08:12:41Z

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

It's not a requirement, just an optimization.

This will need to be documented well - if it's at all useful... The first description I read in this patch made it sound like "off-strategy compaction" is a legitimate alternative to "major compaction", that just behaves a bit differently in a way I didn't understand. Now it has become clear that "off-strategy compaction" only useful after some unspecified "management operations" (which specific operations are those? repair? anything else?), but even that happens even without this command, so what this command apparently does is to expedite this compaction which will happen in 5 minutes anyway. None of this can be understood by the description of that command.

With regular repair, we don't know exactly when the meta-operation is finished since it is managed externally, e.g. by scylla-manager. Therefore, the decision was that we arm a 5 minutes timer that will submit the offstrategy compaction, if scylla is idle and no new repair operation starts.

Maybe the default shouldn't be 5 minutes - that users might find a long boring wait, and look for a command to expedite this. What if, instead, it was a 1 minute delay? 1 minute is more than enough for scylla-manager to start another repair if it wants, but it's not long enough for users to go looking for a command to cancel that delay.

P.S. it's not that I think that there's anything "wrong" with this new API option, it's just that wouldn't it be better if it didn't have to exist, and users wouldn't even need to learn what it means?

@nyh First, I think that offstrategy compaction can be triggered during repair when there are enough sstables already accumulated. By default we limit each compaction job to 32 sstables anyhow so there's no point in accumulating more than that per shard. Then, regarding 1 minute vs. 5 minutes, @asias and @raphaelsc what were these 5 minutes based on?

raphaelsc · 2021-09-28T18:25:52Z

I don't understand then how we can be in a situation that requires the API call. If we did a maintenance operation, it will automatically initiate offstrategy compaction into the main directory.

It's not a requirement, just an optimization.

This will need to be documented well - if it's at all useful... The first description I read in this patch made it sound like "off-strategy compaction" is a legitimate alternative to "major compaction", that just behaves a bit differently in a way I didn't understand. Now it has become clear that "off-strategy compaction" only useful after some unspecified "management operations" (which specific operations are those? repair? anything else?), but even that happens even without this command, so what this command apparently does is to expedite this compaction which will happen in 5 minutes anyway. None of this can be understood by the description of that command.

With regular repair, we don't know exactly when the meta-operation is finished since it is managed externally, e.g. by scylla-manager. Therefore, the decision was that we arm a 5 minutes timer that will submit the offstrategy compaction, if scylla is idle and no new repair operation starts.

Maybe the default shouldn't be 5 minutes - that users might find a long boring wait, and look for a command to expedite this. What if, instead, it was a 1 minute delay? 1 minute is more than enough for scylla-manager to start another repair if it wants, but it's not long enough for users to go looking for a command to cancel that delay.
P.S. it's not that I think that there's anything "wrong" with this new API option, it's just that wouldn't it be better if it didn't have to exist, and users wouldn't even need to learn what it means?

@nyh First, I think that offstrategy compaction can be triggered during repair when there are enough sstables already accumulated. By default we limit each compaction job to 32 sstables anyhow so there's no point in accumulating more than that per shard. Then, regarding 1 minute vs. 5 minutes, @asias and @raphaelsc what were these 5 minutes based on?

@asias picked this. It's not a definitive value, but it's a reasonable one to begin with. Small values may lead to offstrategy running too early and that leads to inefficiency. Even if offstrategy takes a couple of minutes to kick off, that's ok because data from non overlapping sstables are being efficiently served from maintenance set. It's better to delay offstrategy than running it too early.

bhalevy · 2022-01-27T16:14:50Z

Closing in favor of #9980

This series adds methods to perform offstrategy compaction, if needed, returning a future<bool> so the caller can wait on it until compaction completes. The returned value is true iff offstrategy compaction was needed. The added keyspace_offstrategy_compaction calls perform_offstrategy_compaction on the specified keyspace and tables, return the number of tables that required offstrategy compaction. A respective unit test was added to the rest_api pytest. This PR replaces #9095 that suggested adding an option to `keyspace_compaction` since offstrategy compaction triggering logic is different enough from major compaction meriting a new api. Test: unit (dev) Closes #9980 * github.com:scylladb/scylla: test: rest_api: add unit tests for keyspace_offstrategy_compaction api api: add keyspace_offstrategy_compaction compaction_manager: get rid of submit_offstrategy table: add perform_offstrategy_compaction compaction_manager: perform_offstrategy: print ks.cf in log messages compaction_manager: allow waiting on offstrategy compaction

bhalevy requested review from raphaelsc, amnonh, asias and mmatczuk July 26, 2021 11:41

This was referenced Jul 26, 2021

storage_service: forceKeyspaceCompaction: add boolean offStrategy parameter scylladb/scylla-jmx#172

Open

nodetool: compact: add --offstrategy option scylladb/scylla-tools-java#269

Open

bhalevy force-pushed the trigger_offstrategy_compaction branch from 30a2738 to a8f1d89 Compare July 26, 2021 13:02

raphaelsc reviewed Jul 26, 2021

View reviewed changes

api/storage_service.cc Outdated Show resolved Hide resolved

bhalevy force-pushed the trigger_offstrategy_compaction branch from a8f1d89 to 6e0e974 Compare August 1, 2021 11:04

bhalevy changed the title ~~api: add keyspace_trigger_offstrategy_compaction~~ api: add compaction offstrategy option Aug 1, 2021

bhalevy requested a review from raphaelsc August 1, 2021 11:17

raphaelsc suggested changes Aug 4, 2021

View reviewed changes

compaction/compaction_manager.cc Outdated Show resolved Hide resolved

api/storage_service.cc Outdated Show resolved Hide resolved

bhalevy force-pushed the trigger_offstrategy_compaction branch from 6e0e974 to 15dcd18 Compare August 5, 2021 10:28

bhalevy requested a review from raphaelsc August 5, 2021 10:35

raphaelsc approved these changes Aug 5, 2021

View reviewed changes

nyh reviewed Aug 5, 2021

View reviewed changes

database.hh Outdated Show resolved Hide resolved

table.cc Outdated Show resolved Hide resolved

raphaelsc reviewed Aug 5, 2021

View reviewed changes

table.cc Outdated Show resolved Hide resolved

bhalevy force-pushed the trigger_offstrategy_compaction branch 2 times, most recently from a47b7cb to c4f7d29 Compare August 8, 2021 07:54

bhalevy requested review from nyh and raphaelsc August 8, 2021 07:56

nyh approved these changes Aug 8, 2021

View reviewed changes

bhalevy mentioned this pull request Aug 9, 2021

Document offstrategy compaction #9162

Open

bhalevy added 3 commits August 9, 2021 09:38

compaction_manager: perform_offstrategy: print ks.cf in log messages

d18f68c

So it would be easier to relate the messages to the table for which it was submitted. Signed-off-by: Benny Halevy <[email protected]>

bhalevy added 2 commits August 9, 2021 10:45

table: offstrategy compaction: add minimal documentation to api methods

123e407

It is hard to tell which of {trigger,perform,run}_offstrategy_compaction does what. Signed-off-by: Benny Halevy <[email protected]>

bhalevy force-pushed the trigger_offstrategy_compaction branch from c4f7d29 to 123e407 Compare August 9, 2021 08:02

bhalevy mentioned this pull request Jan 27, 2022

Add keyspace_offstrategy_compaction api #9980

Merged

bhalevy closed this Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add compaction offstrategy option #9095

api: add compaction offstrategy option #9095

bhalevy commented Jul 26, 2021 •

edited

Loading

bhalevy commented Aug 5, 2021

raphaelsc left a comment

bhalevy commented Aug 5, 2021

amnonh commented Aug 5, 2021

nyh left a comment

bhalevy commented Aug 8, 2021

nyh left a comment

nyh Aug 8, 2021

nyh Aug 8, 2021

bhalevy Aug 9, 2021

nyh Aug 8, 2021

bhalevy Aug 9, 2021

bhalevy commented Aug 9, 2021

avikivity commented Aug 9, 2021

bhalevy commented Aug 9, 2021

nyh commented Aug 9, 2021

raphaelsc commented Aug 9, 2021 via email

nyh commented Aug 9, 2021

raphaelsc commented Aug 9, 2021 via email

bhalevy commented Aug 9, 2021

bhalevy commented Aug 12, 2021

bhalevy commented Aug 12, 2021

avikivity commented Aug 12, 2021

bhalevy commented Aug 12, 2021

nyh commented Aug 12, 2021

avikivity commented Aug 12, 2021

bhalevy commented Aug 15, 2021

raphaelsc commented Sep 28, 2021

bhalevy commented Jan 27, 2022

api: add compaction offstrategy option #9095

api: add compaction offstrategy option #9095

Conversation

bhalevy commented Jul 26, 2021 • edited Loading

bhalevy commented Aug 5, 2021

raphaelsc left a comment

Choose a reason for hiding this comment

bhalevy commented Aug 5, 2021

amnonh commented Aug 5, 2021

nyh left a comment

Choose a reason for hiding this comment

bhalevy commented Aug 8, 2021

nyh left a comment

Choose a reason for hiding this comment

nyh Aug 8, 2021

Choose a reason for hiding this comment

nyh Aug 8, 2021

Choose a reason for hiding this comment

bhalevy Aug 9, 2021

Choose a reason for hiding this comment

nyh Aug 8, 2021

Choose a reason for hiding this comment

bhalevy Aug 9, 2021

Choose a reason for hiding this comment

bhalevy commented Aug 9, 2021

avikivity commented Aug 9, 2021

bhalevy commented Aug 9, 2021

nyh commented Aug 9, 2021

raphaelsc commented Aug 9, 2021 via email

nyh commented Aug 9, 2021

raphaelsc commented Aug 9, 2021 via email

bhalevy commented Aug 9, 2021

bhalevy commented Aug 12, 2021

bhalevy commented Aug 12, 2021

avikivity commented Aug 12, 2021

bhalevy commented Aug 12, 2021

nyh commented Aug 12, 2021

avikivity commented Aug 12, 2021

bhalevy commented Aug 15, 2021

raphaelsc commented Sep 28, 2021

bhalevy commented Jan 27, 2022

bhalevy commented Jul 26, 2021 •

edited

Loading