Use async compression in kafka client #15920

michael-redpanda · 2024-01-02T21:35:19Z

Fixes: #15900

Utilize asynchronous compression in kafka client to reduce possibility of oversized allocation.

Backports Required

Release Notes

Improvements

Internal kafka client now uses asynchronous compression (when possible) to reduce possibility of oversized allocations and reactor stalls

dotnwat

lgtm

src/v/kafka/client/produce_partition.h

Removing review I realize after seeing Rob's review that I only reviewed the first commit.

src/v/kafka/client/test/produce_partition.cc

graphcareful

LGTM, nice !

src/v/kafka/client/produce_partition.h

michael-redpanda · 2024-01-04T19:54:19Z

CI failure: #15950

BenPope

Looks pretty good.

I wonder if it's worth raising a ticket to opportunistically replace uses of build() with build_async() where the call is already in an async scope.

BenPope · 2024-01-05T13:58:52Z

src/v/storage/record_batch_builder.h

@@ -31,6 +31,7 @@ class record_batch_builder {
      std::optional<iobuf>&& value,
      std::vector<model::record_header> headers);
    virtual model::record_batch build() &&;
+    virtual ss::future<model::record_batch> build_async() &&;


It doesn't look like build() is ever overridden, can these both be made non-virtual to avoid confusion?

BenPope · 2024-01-05T14:16:34Z

src/v/kafka/client/produce_partition.h

+          ssx::spawn_with_gate(
+            _gate, [this]() -> ss::future<> { return try_consume(); });


nitpick:

Suggested change

ssx::spawn_with_gate(

_gate, [this]() -> ss::future<> { return try_consume(); });

ssx::spawn_with_gate(_gate, [this]() { return try_consume(); });

BenPope · 2024-01-05T14:21:10Z

src/v/kafka/client/produce_partition.h

+        if (!consumer_can_run()) {
+            co_return;
        }

-        auto batch_record_count = _config.produce_batch_record_count();
-        auto batch_size_bytes = _config.produce_batch_size_bytes();
+        _consumer(co_await do_consume());
+        co_return;


nitpick (also, what are you doing with this preview, github?):

Suggested change

if (!consumer_can_run()) {

co_return;

}

auto batch_record_count = _config.produce_batch_record_count();

auto batch_size_bytes = _config.produce_batch_size_bytes();

_consumer(co_await do_consume());

co_return;

if (consumer_can_run()) {

_consumer(co_await do_consume());

}

BenPope · 2024-01-05T14:34:18Z

src/v/kafka/client/produce_partition.h

    }

    ss::future<> stop() {
-        try_consume(true);
+        co_await try_consume();


I guess rearming it and then cancelling it isn't too bad. In the case where produce_batch_delay == 0, it may be possible to run another round of try_consume.

To get the original behaviour:

Suggested change

co_await try_consume();

_timer.set_callback([]() {});

co_await try_consume();

BenPope · 2024-01-05T14:38:16Z

src/v/kafka/client/produce_partition.h

+    }
+
+    /// \brief Validates that the size threshold has been met to trigger produce
+    inline bool threshold_met() const {


nitpick: I don't think the inline helps you much here, it's implicitly inline. I suspect you're attempting to hint the compiler to inline the function body, there's a fair chance it will ignore you, as that's not what inline is for.

Yeah good point. I think this was muscle memory from previous work I've done.

BenPope · 2024-01-05T14:38:33Z

src/v/kafka/client/produce_partition.h

+    ///
+    /// Consumer can only run if one is not already running and there are
+    /// records available
+    inline bool consumer_can_run() const {


nitpick: Same comment about inline as above.

michael-redpanda · 2024-01-05T16:57:01Z

Looks pretty good.

I wonder if it's worth raising a ticket to opportunistically replace uses of build() with build_async() where the call is already in an async scope.

#15964

Added async builder for record_batch_builder that can be used to perform asynchronous compression. Signed-off-by: Michael Boquard <michael@redpanda.com>

Updated produce batcher to use async builder that utilizes asynchronous compression. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-01-05T17:04:16Z

Force push d4fc4ac:

Fixups from ben's review

vbotbuildovich · 2024-01-05T19:13:16Z

new failures in https://buildkite.com/redpanda/redpanda/builds/43489#018cdac2-fb91-4f6e-b3b8-df08aec97500:

"rptest.tests.recovery_mode_test.DisablingPartitionsTest.test_disable"

michael-redpanda · 2024-01-05T20:42:26Z

CI:

CI Failure (assert set(hwms4.keys()) == set(orig_hwms.keys())) in DisablingPartitionsTest.test_disable #15972
CI Failure (AssertionError: Expected consumer to be throttled, metric value: 0) in EndToEndThrottlingTest.test_throttling #14139

vbotbuildovich · 2024-01-05T20:52:55Z

/backport v23.3.x

vbotbuildovich · 2024-01-05T20:52:56Z

/backport v23.2.x

vbotbuildovich · 2024-01-05T20:52:57Z

/backport v23.1.x

vbotbuildovich · 2024-01-05T20:53:47Z

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

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-15920-v23.1.x-521 remotes/upstream/v23.1.x
git cherry-pick -x 3a6a7ae6f811bf2237a412abd198c4f3c2871a86 d4fc4acb32d5caf3902cf397021536d7b59e6dc1

Workflow run logs.

vbotbuildovich · 2024-01-05T20:53:50Z

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

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-15920-v23.2.x-645 remotes/upstream/v23.2.x
git cherry-pick -x 3a6a7ae6f811bf2237a412abd198c4f3c2871a86 d4fc4acb32d5caf3902cf397021536d7b59e6dc1

Workflow run logs.

piyushredpanda · 2024-01-08T06:41:47Z

Older branches will need manual cherrypicks, @michael-redpanda. Thanks for the fix, though!

michael-redpanda · 2024-01-09T14:17:23Z

Older branches will need manual cherrypicks, @michael-redpanda. Thanks for the fix, though!

yes, sorry was prioritizing v23.3. Will return to this soon.

michael-redpanda requested review from BenPope, dotnwat and graphcareful January 2, 2024 21:35

github-actions bot added the area/redpanda label Jan 2, 2024

michael-redpanda marked this pull request as ready for review January 2, 2024 21:35

michael-redpanda self-assigned this Jan 2, 2024

dotnwat previously approved these changes Jan 3, 2024

View reviewed changes

graphcareful reviewed Jan 3, 2024

View reviewed changes

src/v/kafka/client/produce_partition.h Outdated Show resolved Hide resolved

src/v/kafka/client/produce_partition.h Outdated Show resolved Hide resolved

src/v/kafka/client/produce_partition.h Outdated Show resolved Hide resolved

graphcareful mentioned this pull request Jan 3, 2024

audit logging perf overhead #15898

Closed

michael-redpanda force-pushed the issues/15900 branch from d1d479d to f698063 Compare January 3, 2024 20:57

michael-redpanda requested review from graphcareful and dotnwat January 3, 2024 20:57

michael-redpanda commented Jan 3, 2024

View reviewed changes

src/v/kafka/client/test/produce_partition.cc Outdated Show resolved Hide resolved

graphcareful previously approved these changes Jan 3, 2024

View reviewed changes

src/v/kafka/client/produce_partition.h Outdated Show resolved Hide resolved

src/v/kafka/client/produce_partition.h Outdated Show resolved Hide resolved

michael-redpanda dismissed graphcareful’s stale review via ea16d32 January 3, 2024 21:25

michael-redpanda force-pushed the issues/15900 branch from f698063 to ea16d32 Compare January 3, 2024 21:25

michael-redpanda requested a review from graphcareful January 3, 2024 21:25

graphcareful previously approved these changes Jan 3, 2024

View reviewed changes

michael-redpanda added this to the v23.3.2 milestone Jan 5, 2024

BenPope reviewed Jan 5, 2024

View reviewed changes

michael-redpanda mentioned this pull request Jan 5, 2024

Opportunistically replace calls to record_batch_builder::build() with record_batch_builder::build_async() #15964

Open

michael-redpanda added 2 commits January 5, 2024 12:01

storage: Added async builder

3a6a7ae

Added async builder for record_batch_builder that can be used to perform asynchronous compression. Signed-off-by: Michael Boquard <michael@redpanda.com>

k/client: Use async compression

d4fc4ac

Updated produce batcher to use async builder that utilizes asynchronous compression. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda dismissed graphcareful’s stale review via d4fc4ac January 5, 2024 17:03

michael-redpanda force-pushed the issues/15900 branch from ea16d32 to d4fc4ac Compare January 5, 2024 17:03

michael-redpanda requested a review from graphcareful January 5, 2024 17:04

michael-redpanda requested a review from BenPope January 5, 2024 17:04

BenPope approved these changes Jan 5, 2024

View reviewed changes

dotnwat merged commit aa00c12 into redpanda-data:dev Jan 5, 2024
16 of 19 checks passed

vbotbuildovich mentioned this pull request Jan 5, 2024

[v23.1.x] Use async compression in kafka client #15973

Closed

This was referenced Jan 5, 2024

[v23.3.x] Oversized allocation: 323584 bytes in compression::stream_zstd::do_compress #15974

Closed

[v23.2.x] Use async compression in kafka client #15975

Open

[v23.3.x] Use async compression in kafka client #15976

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use async compression in kafka client #15920

Use async compression in kafka client #15920

michael-redpanda commented Jan 2, 2024

dotnwat left a comment

graphcareful left a comment

michael-redpanda commented Jan 4, 2024

BenPope left a comment •

edited

BenPope Jan 5, 2024

BenPope Jan 5, 2024

BenPope Jan 5, 2024

BenPope Jan 5, 2024

BenPope Jan 5, 2024

michael-redpanda Jan 5, 2024

BenPope Jan 5, 2024

michael-redpanda commented Jan 5, 2024

michael-redpanda commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

michael-redpanda commented Jan 5, 2024 •

edited

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

piyushredpanda commented Jan 8, 2024

michael-redpanda commented Jan 9, 2024

		ssx::spawn_with_gate(
		_gate, [this]() -> ss::future<> { return try_consume(); });

	ssx::spawn_with_gate(
	_gate, [this]() -> ss::future<> { return try_consume(); });
	ssx::spawn_with_gate(_gate, [this]() { return try_consume(); });

	co_await try_consume();
	_timer.set_callback([]() {});
	co_await try_consume();

Use async compression in kafka client #15920

Use async compression in kafka client #15920

Conversation

michael-redpanda commented Jan 2, 2024

Backports Required

Release Notes

Improvements

dotnwat left a comment

Choose a reason for hiding this comment

graphcareful left a comment

Choose a reason for hiding this comment

michael-redpanda commented Jan 4, 2024

BenPope left a comment • edited

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

michael-redpanda Jan 5, 2024

Choose a reason for hiding this comment

BenPope Jan 5, 2024

Choose a reason for hiding this comment

michael-redpanda commented Jan 5, 2024

michael-redpanda commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

michael-redpanda commented Jan 5, 2024 • edited

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

vbotbuildovich commented Jan 5, 2024

piyushredpanda commented Jan 8, 2024

michael-redpanda commented Jan 9, 2024

BenPope left a comment •

edited

michael-redpanda commented Jan 5, 2024 •

edited