Use opaque records in Raft append entries message #10825
Conversation
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
5 times, most recently
from
2884642 to
f56356e
Compare
mmaslankaprv
changed the title
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
2 times, most recently
from
91b7c31 to
d3d398b
Compare
In addition to using serde, is this a performance boost too? |
Outside of using serde, this limits large allocations (records vector). I think it will provide a boost for workloads involving not compressed batches, haven't measure that yet. |
| @@ -1158,6 +1158,10 @@ class remote_segment_batch_consumer : public storage::batch_consumer { | |||
| // redpanda offset. | |||
| batch.header().base_offset = kafka::offset_cast( | |||
| rp_to_kafka(batch.base_offset())); | |||
| // since base offset isn't accounted into Kafka crc we need to only | |||
| // update header_crc | |||
| batch.header().header_crc = model::internal_header_only_crc( | |||
There was a problem hiding this comment.
is this related to this patch?
There was a problem hiding this comment.
yes, it is as i changed the operator==(record_batch_header) to compare the header_crc
| auto serde_fields() { return std::tie(_header, _records); } | ||
|
|
||
| static model::record_batch | ||
| serde_direct_read(iobuf_parser& in, size_t const bytes_left_limit) { |
There was a problem hiding this comment.
TIL, didn't know about *direct*
There was a problem hiding this comment.
me neither, i like it
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
from
d3d398b to
38984af
Compare
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
from
38984af to
dd7b1f4
Compare
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
3 times, most recently
from
8d49c31 to
c2d8193
Compare
Signed-off-by: Michal Maslanka <michal@redpanda.com>
After batch base offset is updated in remote segment parser we must update the header crc for it to be consistent with current state. Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Serde now supports types without default constructors. Leveraged that and refactored `raft::append_entries_request` type. Signed-off-by: Michal Maslanka <michal@redpanda.com>
Introduced a type wrapping append entries request and defining a new serialization logic for the `append_entries_request`. Leveraging the wrapper approach allow us to decouple type change from its usage and as a result reduce the code churn. Signed-off-by: Michal Maslanka <michal@redpanda.com>
Signed-off-by: Michal Maslanka <michal@redpanda.com>
mmaslankaprv
force-pushed
the
append-entries-allocation
branch
from
c2d8193 to
36def36
Compare
| static ss::future<model::record_batch> | ||
| serde_async_direct_read(iobuf_parser& in, size_t const bytes_left_limit) { | ||
| using serde::read_async_nested; | ||
| // TODO: change to coroutine after we upgrade to clang-16 |
There was a problem hiding this comment.
comment is stale :-D, what are the odds.
Introduced new append entries request type and endpoint. The new type
leverages
serdebased serialization ofmodel::record_batchinrequest
record_batch_reader. The new serialization does notmaterialize batch records but uses an opaque
iobufto serialize batchpayload.
Fixes: #10743
Backports Required
Release Notes
Improvements