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

[v22.3.x] Add an asychronous zstd stream compression class #7850

Merged

Conversation

vbotbuildovich
Copy link
Collaborator

Backport of PR #5565
Fixes #7849,

(cherry picked from commit 9716f66)
…end`

Adding additional scheduling points in `dispatch_send` to account for
the newly futurized `as_scattered` results in a race condition where
messages are sent out of order. As such the easiest solution is to call
`as_scattered` outside of `dispatch_send` where adding a scheduling point
won't result in this race condition. Then pass the result to `dispatch_send`.

(cherry picked from commit c6ca7fd)
Results of performance testing vs the existing `stream_zstd` class are as follows:

```
test                                      iterations      median         mad         min         max      allocs       tasks        inst
streaming_zstd_1mb.compress                     6385   130.290us     1.513us   126.861us   137.173us      16.000       0.000         0.0
streaming_zstd_1mb.uncompress                   4185    78.486us     2.100us    75.342us    80.586us     108.616       0.000         0.0
streaming_zstd_10mb.compress                     653     1.118ms    24.680us     1.041ms     1.142ms     942.830       0.000         0.0
streaming_zstd_10mb.uncompress                   429   844.631us    80.656us   760.340us   976.986us    2095.301       0.000         0.0
async_stream_zstd.1mb_compress                  7616   100.022us   262.374ns    99.547us   100.598us     148.278       2.082         0.0
async_stream_zstd.1mb_uncompress                4804    73.661us   108.821ns    73.542us    73.819us     312.587       6.156         0.0
async_stream_zstd.10mb_compress                  772   979.403us     1.092us   978.266us   985.029us    2176.762      56.239         0.0
async_stream_zstd.10mb_uncompress                490   728.405us   720.304ns   726.690us   731.632us    4089.282     109.756         0.0
```

(cherry picked from commit bb0e700)
(cherry picked from commit 2922bd5)
@jcsp
Copy link
Contributor

jcsp commented Dec 20, 2022

LGTM but needs a clean test run

@BenPope BenPope marked this pull request as ready for review December 22, 2022 21:42
@ballard26
Copy link
Contributor

Failed unit test is test_remote_partition_single_batch_truncated_segments. Some relevant logs lines are;

ERROR 2022-12-19 21:48:10,391 [shard 0] storage - cannot continue parsing. recived size:420 bytes, expected:5211 bytes. context:parser::consume_records
ERROR 2022-12-19 21:48:10,402 [shard 0] storage - cannot continue parsing. recived size:420 bytes, expected:5211 bytes. context:parser::skip_batch
ERROR 2022-12-19 21:48:10,774 [shard 0] storage - cannot continue parsing. recived size:1532 bytes, expected:5325 bytes. context:parser::consume_records
ERROR 2022-12-19 21:48:10,786 [shard 0] storage - cannot continue parsing. recived size:1532 bytes, expected:5325 bytes. context:parser::consume_records
ERROR 2022-12-19 21:54:48,327 [shard 0] cloud_storage - [fiber11373~0|1|982ms] - remote.cc:147 - Unexpected error std::runtime_error (partition_manifest.cc:1096 - Failed to parse topic manifest {"20000000/meta/test-ns/test-topic/42_0/manifest.json"}: Terminate parsing due to Handler error. at offset 262)

The errors look similar to #6054 . But the fix for that issue should be in 22.3.x already

@ballard26
Copy link
Contributor

/ci-repeat 5 debug

@ballard26
Copy link
Contributor

The failure hasn't repeated after 6+ CI runs and appears unrelated to the changes. This should be ready to merge.

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.

4 participants