Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable autokill of segments by default. #12693

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

gianm
Copy link
Contributor

@gianm gianm commented Jun 22, 2022

In PR #12187 the default was changed to druid.coordinator.kill.on = true. However, I think this is too risky of a default without the logic in #12526. Currently the main safeguard against accidental permanent deletion is durationToRetain, which is based on the segment interval, not the time the segment was marked unused. This means older segments can be eligible for autokilling immediately after being marked unused. This 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.

So, I think we should change the default back to false and revisit once the logic in #12526 is implemented. This patch does that, as well as updates the docs to be more clear about how the behavior works, so people can understand it prior to turning autokilling on.

Also add clarifying commentary to the documentation about how durationToRetain works.
@gianm gianm mentioned this pull request Jun 22, 2022
8 tasks
Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm 👍

Should we call out in the 0.23 release notes in a stronger manner that this default has been changed, the implications, and that we are switching it back for the next release?

@gianm
Copy link
Contributor Author

gianm commented Jun 23, 2022

Should we call out in the 0.23 release notes in a stronger manner that this default has been changed, the implications, and that we are switching it back for the next release?

IMO, yes, it would be good for the mention in the 0.23.0 release notes to be more prominent. It's tucked away in the "Coordinator/Overlord" section now, but it's really a bigger change than that. It affects the whole system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants