schemata: use chunked_vector for createable_topic #16529

travisdowns · 2024-02-07T19:26:36Z

Override the decode path to use chunked_vector for createable_topic as we may have 10,000s of these in a single API request.

Arguably, we could use chunked_vector everywhere now in schemata and get rid of this override, but let's do that later.

Fixes #16521.

Backports Required

Release Notes

Bug Fixes

Avoid a large contiguous allocation when creating thousands of topics in a single CreateTopics request.

travisdowns · 2024-02-07T19:29:35Z

src/v/container/include/container/fragmented_vector.h

+     * In the same manner as the corresponding std::vector method.
+     */
+    fragmented_vector(std::initializer_list<value_type> elems)
+      : fragmented_vector(elems.begin(), elems.end()) {}


This is the only new code, the other constructor was just moved to keep all constructors together at the start of the member list.

travisdowns · 2024-02-07T20:13:48Z

src/v/kafka/server/handlers/create_topics.cc

@@ -199,7 +199,8 @@ ss::future<response_ptr> create_topics_handler::handle(
    }

    if (!ctx.audit()) {
-        request.data.topics.erase(valid_range_end, request.data.topics.end());


chunked_vector does not have erase()

I wonder if an chunked_vector::erase_to_end(iterator) would be an easier drop in replacement instead of pop_back_n.

Yes, I had the same thought and this comment pushed me over the the edge to add it, it's in this PR as of the latest push.

travisdowns · 2024-02-07T20:17:27Z

src/v/kafka/server/tests/create_topics_test.cc

@@ -24,7 +24,8 @@
 class create_topic_fixture : public redpanda_thread_fixture {
 public:
    kafka::create_topics_request make_req(
-      std::vector<kafka::creatable_topic> topics, bool validate_only = false) {
+      chunked_vector<kafka::creatable_topic> topics,


The edits in this file are because chunked_vector does not have a copy constructor, so we need to move the request object around now.

This means copying some stuff out here where we used the request after passing it down, and more use of std::move.

rockwotj

LGTM

rockwotj · 2024-02-07T21:07:31Z

src/v/kafka/server/handlers/create_topics.cc

@@ -199,7 +199,8 @@ ss::future<response_ptr> create_topics_handler::handle(
    }

    if (!ctx.audit()) {
-        request.data.topics.erase(valid_range_end, request.data.topics.end());


I wonder if an chunked_vector::erase_to_end(iterator) would be an easier drop in replacement instead of pop_back_n.

mmaslankaprv · 2024-02-08T11:42:52Z

src/v/container/include/container/fragmented_vector.h

+     *
+     * This has the same semantics as the corresponding std::vector
+     * constructor.
+     */
    template<typename Iter>
    requires std::input_iterator<Iter>
    fragmented_vector(Iter begin, Iter end)


nit: would it be possible to change commit message to keep the naming consistent i.e. fragemented_vector ?

Sure, updated to use fragmented_vector.

We add a std::initalizer_list constructor to fragmented_vector. This is present in std::vector making some swap-outs more difficult.

rockwotj

LGTM

src/v/container/include/container/fragmented_vector.h

This is similar to pop_back_n but takes an iterator and is a more directly replacement for use cases which are already using iterators. Additionally, allow const_iterator to be converted to iterator, which is what standard containers do and is important for ergonomics.

Override the decode path to use chunked_vector for creatable_topic as we may have 10,000s of these in a single API request. Arguably, we could use chunked_vector everywhere now in schemata and get rid of this override, but let's do that later. Fixes redpanda-data#16521.

vbotbuildovich · 2024-02-08T23:29:32Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44914#018d8ac1-b3d6-4ab1-a1e3-f38e4139937c

vbotbuildovich · 2024-02-09T01:56:33Z

/backport v23.3.x

vbotbuildovich · 2024-02-09T01:57:33Z

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16529-v23.3.x-907 remotes/upstream/v23.3.x
git cherry-pick -x 0eaea00d9f8f034fc2177540cc88266543c64f00 5e5b4e885e5104ae7bc958dc6b88601320946f30 51fda27d0da756376929f2c2b864f8b186dd91d1

Workflow run logs.

travisdowns · 2024-02-16T19:58:07Z

/backport v23.3.x

travisdowns requested a review from michael-redpanda February 7, 2024 19:26

github-actions bot added the area/redpanda label Feb 7, 2024

travisdowns requested a review from ballard26 February 7, 2024 19:26

travisdowns commented Feb 7, 2024

View reviewed changes

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from e99567b to 879ae45 Compare February 7, 2024 20:12

travisdowns commented Feb 7, 2024

View reviewed changes

rockwotj previously approved these changes Feb 7, 2024

View reviewed changes

mmaslankaprv reviewed Feb 8, 2024

View reviewed changes

travisdowns dismissed rockwotj’s stale review via a2bff73 February 8, 2024 13:01

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from 879ae45 to a2bff73 Compare February 8, 2024 13:01

Add initializer list constructor to frag_vector

0eaea00

We add a std::initalizer_list constructor to fragmented_vector. This is present in std::vector making some swap-outs more difficult.

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from a2bff73 to 1377c44 Compare February 8, 2024 14:43

travisdowns requested review from mmaslankaprv and rockwotj February 8, 2024 14:59

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from 1377c44 to bb23a02 Compare February 8, 2024 15:35

rockwotj previously approved these changes Feb 8, 2024

View reviewed changes

src/v/container/include/container/fragmented_vector.h Outdated Show resolved Hide resolved

travisdowns dismissed rockwotj’s stale review via fda1625 February 8, 2024 16:51

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from bb23a02 to fda1625 Compare February 8, 2024 16:51

rockwotj previously approved these changes Feb 8, 2024

View reviewed changes

travisdowns dismissed rockwotj’s stale review via 51fda27 February 8, 2024 20:30

travisdowns force-pushed the td-16521-chunked-vector-create-topics branch from fda1625 to 51fda27 Compare February 8, 2024 20:30

rockwotj approved these changes Feb 8, 2024

View reviewed changes

travisdowns merged commit 02ef9fb into redpanda-data:dev Feb 9, 2024
18 checks passed

vbotbuildovich mentioned this pull request Feb 9, 2024

[v23.3.x] schemata: use chunked_vector for createable_topic #16553

Closed

travisdowns mentioned this pull request Feb 9, 2024

[v23.3.x] schemata: use chunked_vector for createable_topic #16557

Closed

travisdowns mentioned this pull request Feb 16, 2024

[v23.3.x] schemata: use chunked_vector for createable_topic #16620

Closed

This was referenced Feb 16, 2024

[v23.3.x] Redpanda reports oversized allocation when trying to create topics near the partition limit #16621

Closed

[v23.3.x] schemata: use chunked_vector for createable_topic #16622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemata: use chunked_vector for createable_topic #16529

schemata: use chunked_vector for createable_topic #16529

travisdowns commented Feb 7, 2024

travisdowns Feb 7, 2024

travisdowns Feb 7, 2024

rockwotj Feb 7, 2024

travisdowns Feb 8, 2024

travisdowns Feb 7, 2024

rockwotj left a comment

rockwotj Feb 7, 2024

mmaslankaprv Feb 8, 2024

travisdowns Feb 8, 2024

rockwotj left a comment

vbotbuildovich commented Feb 8, 2024

vbotbuildovich commented Feb 9, 2024

vbotbuildovich commented Feb 9, 2024

travisdowns commented Feb 16, 2024

schemata: use chunked_vector for createable_topic #16529

schemata: use chunked_vector for createable_topic #16529

Conversation

travisdowns commented Feb 7, 2024

Backports Required

Release Notes

Bug Fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockwotj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockwotj left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Feb 8, 2024

vbotbuildovich commented Feb 9, 2024

vbotbuildovich commented Feb 9, 2024

travisdowns commented Feb 16, 2024