Enable auto kill segments by default #12187
Conversation
| return killUnusedSegmentsInAllDataSources != null | ||
| ? killUnusedSegmentsInAllDataSources | ||
| : specificDataSourcesToKillUnusedSegmentsIn.isEmpty(); |
There was a problem hiding this comment.
Can this be simplified as?
| return killUnusedSegmentsInAllDataSources != null | |
| ? killUnusedSegmentsInAllDataSources | |
| : specificDataSourcesToKillUnusedSegmentsIn.isEmpty(); | |
| return specificDataSourcesToKillUnusedSegmentsIn.isEmpty(); |
The method returns true when
- either
killUnusedSegmentsInAllDataSources = truewhich implies thatspecifiedDataSourcesToKillUnusedSegmentsInis empty, otherwise we would have thrown an exception - or
killUnusedSegmentsInAllDataSources = nullandspecifiedDataSourcesToKillUnusedSegmentsInis empty
With these new defaults, maybe we don't even need killAll anymore?
If killList is empty, we kill all
Otherwise, we kill only the ones specified.
Unless I am missing a case.
There was a problem hiding this comment.
Great suggestion. This should simplify the user experience. I will do this
| @@ -65,7 +65,8 @@ | |||
| /** | |||
| * If true {@link KillUnusedSegments} sends kill tasks for unused segments in all data sources. | |||
| */ | |||
| private final boolean killUnusedSegmentsInAllDataSources; | |||
| @Nullable | |||
| private final Boolean killUnusedSegmentsInAllDataSources; | |||
There was a problem hiding this comment.
nit: can we compute its default per the given killDataSourceWhitelist instead of making it nullable? It could simplify the code a bit.
|
Can you set druid_coordinator_kill_period=PT360000S in integration-tests/docker/environment-configs/coordinator too? Thanks |
|
@kfaraz I should have removed all references to |
|
IMO without the logic in #12526, enabling autokill by default is too risky. Marking segments unused can happen by mistake: someone might activate the "mark unused" functionality (via API or web console) for the wrong datasource, or the wrong interval. Then autokilling can turn that small mistake into a big mistake. I raised a PR to set it back: #12693 |
This parameter has been removed for awhile now as of Druid 0.23.0 apache#12187. The code was only used in tests to verify that serialization works. Now remove all references to avoid any confusion.
…ces. (apache#16387) This parameter has been removed for awhile now as of Druid 0.23.0 apache#12187. The code was only used in tests to verify that serialization works. Now remove all references to avoid any confusion.
Description
To help prevent the metadata store from growing unbounded, enable kill unused segments by default on all datasources.
This is helpful when users have auto-compaction running or they age out data. The old segments are marked as unused but live on in the metadata store and in deep storage.
The new defaults should kill all unused segments older than 90 days. If users do not want this behavior on an upgrade, they should explicitly disable the behavior, or change the specific datasources they would want to run on.
killAllDataSourcesis removed and is no longer respected. This means users who are upgrading with kill.on set to true, butkillAllDataSourcesset to false ANDkillDataSourceWhitelistset to empty, will start to see unused segments older than 90 days being killed after an upgrade that were not being deleted before the upgrade.This seems like a reasonable risk - since it is a strange combination to have kill enabled via kill.on, but then disabled killing all datasources as described above. Based on this I think this is a reasonable thing to call out in the release notes to give operators a heads up of this edge case, since killing segments deletes them from deep storage as well.
This PR has: