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

Overrides for space_management_enable and retention_local_strict #15761

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented Dec 19, 2023

This PR works around an issue where certain configs could be impossible to move durably off their legacy defaults after an upgrade by providing override config properties without legacy defaults (and with default values matching the legacy default value).

Fixes #15674

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

Bug Fixes

  • Fix an issue where new configs would continually revert to legacy defaults after an upgrade.

dotnwat
dotnwat previously approved these changes Dec 19, 2023
@oleiman oleiman requested review from dotnwat and mmaslankaprv and removed request for mmaslankaprv December 19, 2023 07:27
mmaslankaprv
mmaslankaprv previously approved these changes Dec 19, 2023
@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/43037#018c8147-19ee-4987-8f3d-8a564f678cab:

"rptest.tests.partition_balancer_test.PartitionBalancerTest.test_nodes_with_reclaimable_space"

Comment on lines 1013 to 1014
!config::shard_local_cfg().retention_local_strict()
&& !config::shard_local_cfg().retention_local_strict_override()) {
Copy link
Member

Choose a reason for hiding this comment

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

i think the test is failing as now we must set both properties if we do not want to use strict local retention, is that expected ?

Copy link
Member Author

@oleiman oleiman Dec 19, 2023

Choose a reason for hiding this comment

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

My mistake, I believe it should be one or the other.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman
Copy link
Member Author

oleiman commented Dec 19, 2023

force push to fix incorrect retention_local_strict_override condition

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

We need to make sure that we create a ticket to update documentation procedure for upgrading and how to get these two legacy defaults to stick by setting the new config options.

@oleiman oleiman merged commit 691b4c8 into redpanda-data:dev Dec 19, 2023
19 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

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

Successfully merging this pull request may close these issues.

Properties revert back to default value upon restart, upgrade related
4 participants