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

[v23.3.x] config/configuration: set cloud_storage_spillover_manifest_size #16174

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented Jan 19, 2024

Backport of PR #16172

this enable splitting the partition_manifest into spillovers once it
reaches 2*cloud_storage_spillover_manifest_size

(cherry picked from commit f4f2581)
@andijcr andijcr added the kind/backport PRs targeting a stable branch label Jan 19, 2024
@andijcr andijcr requested a review from Lazin January 19, 2024 13:48
@andijcr andijcr added this to the v23.3.3 milestone Jan 19, 2024
@andijcr
Copy link
Contributor Author

andijcr commented Jan 19, 2024

test failures are probably related to the changed default. investigating...

fix use of cloud_storage_spillover_manifest_max_segments
explicitly disable cloud_storage_spillover_manifest_size if the tests
bootstraps with cloud_storage_spillover_manifest_max_segments.

this is done because inside redpanda, size takes precendece over
max_segments and the tests will work with wrong assumptions
redpanda ignores cloud_storage_spillover_manifest_max_segments when
cloud_storage_spillover_manifest_size is set. explicitly set the latter
to None
@andijcr
Copy link
Contributor Author

andijcr commented Jan 19, 2024

/ci-repeat 1 skip-unit skip-redpanda-build

@andijcr
Copy link
Contributor Author

andijcr commented Jan 19, 2024

from last build, the known issues are #15893 and #15489. the other should be fixed by the last commit

Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

LGTM, pending CI

Partition shutdown and purger run asynchronously and can race since
\redpanda-data#11873. 5ms grace period for tests is too as TS requests for final
manifest upload can take many milliseconds, especially with real
TS backend.

Fixes redpanda-data#13510
@andrwng
Copy link
Contributor

andrwng commented Jan 20, 2024

Also pulled in #15814 since there was a conflict and it's a one-line CI flake fix

These tests either weren't designed to use spillover, or were manually
configuring a max segment count. In either case, the new default value
is problematic for them since the size-based config takes precedence.

I went through all tests I could find that configured
cloud_storage_spillover_manifest_max_segments and unset
cloud_storage_spillover_manifest_size as appropriate.

Previous attempts to alleviate this attempted to set these at runtime
with the incorrect ducktape methods (set_cluster_config() instead of
set_cluster_config_to_null()). It's more foolproof to just set them at
bootstrap time, as they were set before the default was introduced.
@andrwng andrwng force-pushed the vbotbuildovich/backport-16172-v23.3.x-282 branch from de4830a to 9290244 Compare January 20, 2024 01:41
@piyushredpanda
Copy link
Contributor

/dt

@piyushredpanda piyushredpanda merged commit 74d4da8 into redpanda-data:v23.3.x Jan 20, 2024
15 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redpanda kind/backport PRs targeting a stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v23.3.x] config/configuration: set cloud_storage_spillover_manifest_size
4 participants