config/configuration: set cloud_storage_spillover_manifest_size #16172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this enable splitting the partition_manifest into spillovers once it reaches 2*cloud_storage_spillover_manifest_size
|
/backport v23.3.x |
|
The pull request is not merged yet. Cancelling backport... |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2233-63b6-463c-99fc-f7d1e07dd017: new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2239-58c9-46ac-a705-c9a449b638a3: new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2233-63bf-4f5b-bf67-d6c2ef3d1bf7: new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2233-63ba-45db-bf0e-4418021975eb: new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2239-58c3-4560-8ee0-93675544e565: new failures in https://buildkite.com/redpanda/redpanda/builds/43945#018d2233-63bc-46b6-ac69-2001f948edbb: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2348-208d-4f36-96d4-e7f9237a8caa: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2348-2093-42e6-a8b6-9388c0b2896e: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2355-f163-464f-a4ef-c7a7e12f2dad: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2355-f16d-4828-a365-9b0ffd7d7f32: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2355-f167-4548-8f35-c9905e4cef1b: new failures in https://buildkite.com/redpanda/redpanda/builds/43979#018d2355-f16a-4695-bc5c-74c4e59f91a0: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d181-40a3-b3fb-56bd84e9b92d: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d17a-4977-83c4-040227117fe3: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d188-4efb-8a40-4e807c88b5f6: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d192-4fad-b373-a722fc0e05ee: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d18b-4a33-81d5-48315a2c6a74: new failures in https://buildkite.com/redpanda/redpanda/builds/44001#018d23b3-d18f-4533-821b-6e05d8bd3516: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c0-775d-4cb9-af67-b6fe3a1d8550: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c0-7768-4c15-9fc2-71e8ffceff44: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c5-48b8-49d7-9bbe-38749f65d8cd: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c0-7761-4ffa-aad5-ad7ce010596f: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c5-48b2-4ec2-8a25-19cfcdb16bc3: new failures in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c0-7765-41cc-927a-28291d7a9719: |
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
|
test fixes: explicitly disable cloud_storage_spillover_manifest_size if the test uses cloud_storage_spillover_manifest_max_segments. this is because inside redpanda size takes precedence over max_segments and the test fails due to wrong assumptions |
andrwng
reviewed
Jan 19, 2024
Comment on lines
+3242
to
+3248
| # in redpanda, cloud_storage_spillover_manifest_size takes preference over cloud_storage_spillover_manifest_max_segments. | ||
| # disable the former to use the latter | ||
| assert conf.get('cloud_storage_spillover_manifest_size', None) is None or \ | ||
| conf.get('cloud_storage_spillover_manifest_max_segments', None) is None, \ | ||
| "cannot set cloud_storage_spillover_manifest_max_segments if cloud_storage_spillover_manifest_size is already set, it will not be used by redpanda" | ||
| if 'cloud_storage_spillover_manifest_max_segments' in conf: | ||
| conf['cloud_storage_spillover_manifest_size'] = None |
There was a problem hiding this comment.
This is surprising, and is something I can imagine us tripping over in the future.
Why isn't it sufficient to just set cloud_storage_spillover_manifest_size to None in the tests that set ..max_segments?
There was a problem hiding this comment.
Some tests set the value in extra_rp_conf, some other set it in SISettings, i thought to just fix it in one place
There was a problem hiding this comment.
some other related tests are failing on the backport, I'll fix what there but just skip the redpanda build in the ci
There was a problem hiding this comment.
I'm also curious about this. How many tests does this affect can we just update a couple tests explicitly?
There was a problem hiding this comment.
Got it. It is a little annoying, but I think it's preferable since it's less surprising than having this business logic encoded in the service itself. For instance if we wanted to test these two configs together, which isn't an unreasonable thing for an unsuspecting user to do, we wouldn't be able to now
andrwng
previously approved these changes
Jan 19, 2024
redpanda ignores cloud_storage_spillover_manifest_max_segments when cloud_storage_spillover_manifest_size is set. explicitly set the latter to None
|
/ci-repeat 1 skip-unit skip-redpanda-build |
Agreed if we create tickets to track the follow up work. |
andrwng
previously approved these changes
Jan 19, 2024
Closed
andrwng
reviewed
Jan 19, 2024
| @@ -262,7 +262,8 @@ def __init__(self, test_context): | |||
| cloud_storage_segment_size_min=2 * self.log_segment_size, | |||
| retention_local_target_bytes_default=2 * self.log_segment_size, | |||
| cloud_storage_enable_segment_merging=True, | |||
| cloud_storage_cache_chunk_size=self.chunk_size) | |||
| cloud_storage_cache_chunk_size=self.chunk_size, | |||
| cloud_storage_spillover_manifest_size=None) | |||
There was a problem hiding this comment.
I suspect setting this to none will actually just not write the config, rather than setting it to nullopt.
There was a problem hiding this comment.
Nevermind. I was looking at the wrong test results :)
andrwng
force-pushed
the
feat/enable_spillover_by_default
branch
from
440a9ef
to
b03f5af
Compare
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
force-pushed
the
feat/enable_spillover_by_default
branch
from
b03f5af
to
70199a8
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44018#018d24c0-7761-4ffa-aad5-ad7ce010596f |
dotnwat
approved these changes
Jan 20, 2024
|
/backport v23.3.x |
|
Failed to create a backport PR to v23.3.x branch. I tried: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this enable splitting the partition_manifest into spillovers once it reaches 2*cloud_storage_spillover_manifest_size
Fixes https://github.com/redpanda-data/core-internal/issues/1012
Backports Required
Release Notes
Features