cloud_storage/tests: use better containers in s3_imposter #29512
Conversation
This file contains hidden or 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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces standard library containers in s3_imposter with more memory-efficient alternatives to prevent allocation failures during high-volume testing. The motivation stems from a test failure where ~800KB could not be allocated due to container growth in s3_imposter.
Changes:
- Replaced
std::vectorwithchunked_vectorfor various collections in s3_imposter and related test utilities - Replaced
std::map/std::multimapwithabsl::btree_map/absl::btree_multimapfor ordered collections - Updated function signatures and call sites throughout test code to use the new container types
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/cloud_io/tests/s3_imposter.h | Updated s3_imposter interface to use chunked_vector and absl::btree_multimap |
| src/v/cloud_io/tests/s3_imposter.cc | Implemented container type changes in s3_imposter, replacing std containers |
| src/v/cloud_io/tests/BUILD | Added dependencies for chunked_vector and absl::btree containers |
| src/v/cloud_storage/tests/util.h | Changed return types of make_imposter_expectations to chunked_vector |
| src/v/cloud_storage/tests/util.cc | Updated implementations to use chunked_vector and std::move semantics |
| src/v/cloud_storage/tests/BUILD | Added chunked_vector dependency |
| src/v/cloud_storage/tests/remote_test.cc | Changed local variables to const references to avoid copies |
| src/v/cloud_storage/tests/remote_partition_test.cc | Updated expectation handling to use chunked_vector with std::move |
| src/v/cloud_storage/tests/async_manifest_view_test.cc | Changed _expectations field to chunked_vector and added std::move |
| src/v/cloud_storage/tests/topic_mount_handler_test.cc | Refactored expectation construction to use chunked_vector |
| src/v/datalake/coordinator/tests/iceberg_snapshot_remover_test.cc | Updated remove_expectations call to use chunked_vector::single |
| src/v/iceberg/tests/table_io_test.cc | Updated add_expectations call to use chunked_vector::single |
| src/v/lsm/io/tests/persistence_test.cc | Updated remove_expectations call to use chunked_vector::single |
Comment on lines
+593
to
+599
| // Build new vector excluding manifest expectation | ||
| chunked_vector<cloud_storage_fixture::expectation> expectations; | ||
| for (auto& e : all_expectations) { | ||
| if (e.url != manifest_url) { | ||
| expectations.push_back(std::move(e)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop constructs a new chunked_vector by copying/moving elements from all_expectations except the manifest. This is less efficient than removing the manifest element directly. Consider using std::remove_if or erase-remove idiom on all_expectations instead of creating a new vector.
| s3_imposter_fixture::req_pred_t predicate) const { | ||
| std::vector<http_test_utils::request_info> matching_requests; | ||
| matching_requests.reserve(_requests.size()); | ||
| chunked_vector<http_test_utils::request_info> matching_requests; |
There was a problem hiding this comment.
The reserve() call was removed when converting from std::vector to chunked_vector. While chunked_vector has different memory characteristics, pre-allocating capacity when the upper bound is known (_requests.size()) could still improve performance by reducing reallocations.
Suggested change
| chunked_vector<http_test_utils::request_info> matching_requests; | |
| chunked_vector<http_test_utils::request_info> matching_requests; | |
| matching_requests.reserve(_requests.size()); |
Saw an instance where we failed to allocate ~800KB because one of the containers in s3_imposters grew to be large. It isn't unreasonable to expect a test to bombard s3_imposter with requests, especially when trying to stress certain subsystems. This replaces the containers in s3_imposter with chunked_vector and absl::btree_multimap, which are more aligned with best practices of our codebase.
Saw one instance of this test using ~1GB which contributed to it crashing.
andrwng
force-pushed
the
s3-imposter-chunked
branch
from
2b04ff8 to
faf9722
Compare
Collaborator
CI test resultstest results on build#80060
|
dotnwat
approved these changes
Feb 4, 2026
| } | ||
|
|
||
| void listen() { set_expectations_and_listen(_expectations); } | ||
| void listen() { set_expectations_and_listen(std::move(_expectations)); } |
Member
There was a problem hiding this comment.
this ok? just happens once and then no more uses of expectations member var?
Contributor
Author
There was a problem hiding this comment.
Yea, this is only used to set up expectations at the beginning of certain tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Saw an instance where we failed to allocate ~800KB because one of the containers in s3_imposters grew to be large. It isn't unreasonable to expect a test to bombard s3_imposter with requests, especially when trying to stress certain subsystems.
This replaces the containers in s3_imposter with chunked_vector and absl::btree_multimap, which are more aligned with best practices of our codebase.
Backports Required
Release Notes