Skip to content

Serde: do not use reflection #29156

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.

Sign up for GitHub

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

Merged
bashtanov merged 2 commits into from
Jan 8, 2026
Merged

Serde: do not use reflection #29156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e7b2b5
cloud_storange: base all serde classes on serde::envelope
bashtanov Jan 6, 2026
1177b3d
serde: do not use reflection, require classes to inherit envelope
bashtanov Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/v/cloud_storage/segment_meta_cstore.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class column_store
};

static_assert(
reflection::arity<segment_meta>()
std::tuple_size_v<decltype(std::declval<segment_meta>().serde_fields())>
== std::tuple_size_v<decltype(segment_meta_accessors)>,
"segment_meta has a field that is not in segment_meta_accessors. check "
"also that the members of column_store match the members of "
Expand Down
16 changes: 6 additions & 10 deletions src/v/cloud_storage/tests/partition_manifest_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,11 +1544,9 @@ struct metadata_stm_segment
};

namespace old {
struct segment_meta_v0 {
using value_t = segment_meta_v0;
static constexpr serde::version_t redpanda_serde_version = 0;
static constexpr serde::version_t redpanda_serde_compat_version = 0;

struct segment_meta_v0
: serde::
envelope<segment_meta_v0, serde::version<0>, serde::compat_version<0>> {
bool is_compacted;
size_t size_bytes;
model::offset base_offset;
Expand All @@ -1570,11 +1568,9 @@ struct segment_meta_v0 {

auto operator<=>(const segment_meta_v0&) const = default;
};
struct segment_meta_v1 {
using value_t = segment_meta_v1;
static constexpr serde::version_t redpanda_serde_version = 1;
static constexpr serde::version_t redpanda_serde_compat_version = 0;

struct segment_meta_v1
: serde::
envelope<segment_meta_v1, serde::version<1>, serde::compat_version<0>> {
bool is_compacted;
size_t size_bytes;
model::offset base_offset;
Expand Down
24 changes: 19 additions & 5 deletions src/v/cloud_storage/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,8 @@ struct offset_range {
model::offset end_rp;
};

struct segment_meta {
using value_t = segment_meta;
static constexpr serde::version_t redpanda_serde_version = 3;
static constexpr serde::version_t redpanda_serde_compat_version = 0;

struct segment_meta
: serde::envelope<segment_meta, serde::version<3>, serde::compat_version<0>> {
bool is_compacted;
size_t size_bytes;
model::offset base_offset;
Expand All @@ -111,6 +108,23 @@ struct segment_meta {
/// Size of the metadata (optional)
uint64_t metadata_size_hint{0};

auto serde_fields() {
return std::tie(
is_compacted,
size_bytes,
base_offset,
committed_offset,
base_timestamp,
max_timestamp,
delta_offset,
ntp_revision,
archiver_term,
segment_term,
delta_offset_end,
sname_format,
metadata_size_hint);
}

kafka::offset base_kafka_offset() const {
// Manifests created with the old version of redpanda won't have the
// delta_offset field. In this case the value will be initialized to
Expand Down
2 changes: 0 additions & 2 deletions src/v/serde/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ redpanda_cc_library(
deps = [
"//src/v/bytes:iobuf_parser",
"//src/v/hashing:crc32c",
"//src/v/reflection:arity",
"//src/v/reflection:to_tuple",
"//src/v/ssx:sformat",
],
)
Expand Down
8 changes: 1 addition & 7 deletions src/v/serde/envelope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,10 @@ inline constexpr std::size_t checksum_envelope_header_size
= envelope_header_size + 4;

template<typename T, typename Version = const serde::version_t&>
concept is_envelope = requires {
{ T::redpanda_serde_version } -> std::same_as<Version>;
{ T::redpanda_serde_compat_version } -> std::same_as<Version>;
};
concept is_envelope = T::redpanda_inherits_from_envelope;

template<typename T>
concept is_checksum_envelope = is_envelope<T>
&& T::redpanda_serde_build_checksum;

template<typename T>
concept inherits_from_envelope = T::redpanda_inherits_from_envelope;

} // namespace serde
21 changes: 3 additions & 18 deletions src/v/serde/envelope_for_each_field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

#pragma once

#include "reflection/arity.h"
#include "reflection/to_tuple.h"
#include "serde/envelope.h"

#include <tuple>
Expand All @@ -29,26 +27,13 @@ concept check_for_more_fn = requires(Fn&& fn, int& f) {

template<is_envelope T, typename Fn>
inline auto envelope_for_each_field(T& t, Fn&& fn) {
if constexpr (inherits_from_envelope<std::decay_t<T>>) {
std::apply(
[&](auto&&... args) { (fn(args), ...); }, envelope_to_tuple(t));
} else {
std::apply(
[&](auto&&... args) { (fn(args), ...); }, reflection::to_tuple(t));
}
std::apply([&](auto&&... args) { (fn(args), ...); }, envelope_to_tuple(t));
Copy link
Member

@dotnwat dotnwat Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serde: do not use reflection, require classes to inherit envelope

i don't think we have to inherit from envelope, right? i'm pretty sure we have some types that manually add the same metadata that the envelop would otherwise inject. if you want to make this a requirement, then maybe we need a static assert? or is there one?

Copy link
Contributor Author

@bashtanov bashtanov Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we have to inherit from envelope, right?

This PR makes it a requirement

if you want to make this a requirement, then maybe we need a static assert? or is there one?

tag_invoke for both read_tag and write_tag still requires is_envelope<std::decay_t<T>>, which now checks class inheritance instead of a duck-typing-style check

i'm pretty sure we have some types that manually add the same metadata that the envelop would otherwise inject.

The first commit of this PR changes them all to inherit from envelope.

Copy link
Member

@oleiman oleiman Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duck-typing-style check

yeah i don't really get why we supported this in the first place. some holdover from the ADL -> serde transition or something?

Copy link
Member

@dotnwat dotnwat Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

}

template<is_envelope T, check_for_more_fn Fn>
inline auto envelope_for_each_field(T& t, Fn&& fn) {
if constexpr (inherits_from_envelope<std::decay_t<T>>) {
std::apply(
[&](auto&&... args) { (void)(fn(args) && ...); },
envelope_to_tuple(t));
} else {
std::apply(
[&](auto&&... args) { (void)(fn(args) && ...); },
reflection::to_tuple(t));
}
std::apply(
[&](auto&&... args) { (void)(fn(args) && ...); }, envelope_to_tuple(t));
}

} // namespace serde
7 changes: 5 additions & 2 deletions src/v/serde/test/fuzz.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,16 @@ bool eq(
return ((std::get<I>(a) == std::get<I>(b)) && ...);
}

template<serde::is_envelope E>
constexpr size_t arity
= std::tuple_size_v<decltype(envelope_to_tuple(std::declval<E>()))>;

template<serde::is_envelope T1, serde::is_envelope T2>
bool operator==(T1 const& a, T2 const& b) {
return eq(
envelope_to_tuple(a),
envelope_to_tuple(b),
std::make_index_sequence<std::min(
reflection::arity<T1>() - 1, reflection::arity<T2>() - 1)>());
std::make_index_sequence<std::min(arity<T1>, arity<T2>)>());
}

struct data_gen {
Expand Down
64 changes: 20 additions & 44 deletions src/v/serde/test/serde_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,23 +117,9 @@ struct test_msg1_new
auto serde_fields() { return std::tie(_a, _m, _b, _c); }
};

struct test_msg1_new_manual {
using value_t = test_msg1_new_manual;
static constexpr auto redpanda_serde_version = serde::version_t{10};
static constexpr auto redpanda_serde_compat_version = serde::version_t{5};

bool operator==(const test_msg1_new_manual&) const = default;

int _a;
test_msg0 _m;
int _b, _c;
};

struct not_an_envelope {};
static_assert(!serde::is_envelope<not_an_envelope>);
static_assert(serde::is_envelope<test_msg1>);
static_assert(serde::inherits_from_envelope<test_msg1_new>);
static_assert(!serde::inherits_from_envelope<test_msg1_new_manual>);
static_assert(test_msg1::redpanda_serde_version == 4);
static_assert(test_msg1::redpanda_serde_compat_version == 0);

Expand All @@ -143,16 +129,6 @@ SEASTAR_THREAD_TEST_CASE(incompatible_version_throws) {
serde::serde_exception);
}

SEASTAR_THREAD_TEST_CASE(manual_and_envelope_equal) {
const auto roundtrip = serde::from_iobuf<test_msg1_new_manual>(
serde::to_iobuf(
test_msg1_new{
._a = 77, ._m = test_msg0{._i = 2, ._j = 3}, ._b = 88, ._c = 99}));
const auto check = test_msg1_new_manual{
._a = 77, ._m = test_msg0{._i = 2, ._j = 3}, ._b = 88, ._c = 99};
BOOST_CHECK(roundtrip == check);
}

SEASTAR_THREAD_TEST_CASE(reserve_test) {
auto b = iobuf();
auto p = b.reserve(10);
Expand Down Expand Up @@ -648,17 +624,17 @@ SEASTAR_THREAD_TEST_CASE(serde_checksum_envelope_test) {
serde::compat_version<2>> {
bool operator==(const checksummed&) const = default;

std::vector<test_msg1_new_manual> data_;
std::vector<test_msg1_new> data_;
auto serde_fields() { return std::tie(data_); }
};

const auto obj = checksummed{
.data_ = {
test_msg1_new_manual{
test_msg1_new{
._a = 1, ._m = test_msg0{._i = 33, ._j = 44}, ._b = 2, ._c = 3},
test_msg1_new_manual{
test_msg1_new{
._a = 4, ._m = test_msg0{._i = 55, ._j = 66}, ._b = 5, ._c = 6},
test_msg1_new_manual{
test_msg1_new{
._a = 7, ._m = test_msg0{._i = 77, ._j = 88}, ._b = 8, ._c = 9}}};
const auto vec = std::vector<checksummed>{obj, obj};
BOOST_CHECK(
Expand All @@ -670,7 +646,7 @@ struct old_no_cs
envelope<old_no_cs, serde::version<3>, serde::compat_version<2>> {
bool operator==(const old_no_cs&) const = default;

std::vector<test_msg1_new_manual> data_;
std::vector<test_msg1_new> data_;
auto serde_fields() { return std::tie(data_); }
};
struct new_cs
Expand All @@ -696,27 +672,27 @@ struct new_cs

bool operator==(const new_cs&) const = default;

std::vector<test_msg1_new_manual> data_;
std::vector<test_msg1_new> data_;
auto serde_fields() { return std::tie(data_); }
};

SEASTAR_THREAD_TEST_CASE(serde_checksum_update) {
const auto old_obj = old_no_cs{
.data_ = {
test_msg1_new_manual{
test_msg1_new{
._a = 1, ._m = test_msg0{._i = 33, ._j = 44}, ._b = 2, ._c = 3},
test_msg1_new_manual{
test_msg1_new{
._a = 4, ._m = test_msg0{._i = 55, ._j = 66}, ._b = 5, ._c = 6},
test_msg1_new_manual{
test_msg1_new{
._a = 7, ._m = test_msg0{._i = 77, ._j = 88}, ._b = 8, ._c = 9}}};

const auto new_obj = new_cs{
.data_ = {
test_msg1_new_manual{
test_msg1_new{
._a = 1, ._m = test_msg0{._i = 33, ._j = 44}, ._b = 2, ._c = 3},
test_msg1_new_manual{
test_msg1_new{
._a = 4, ._m = test_msg0{._i = 55, ._j = 66}, ._b = 5, ._c = 6},
test_msg1_new_manual{
test_msg1_new{
._a = 7, ._m = test_msg0{._i = 77, ._j = 88}, ._b = 8, ._c = 9}}};

const auto old_vec = std::vector<old_no_cs>{old_obj, old_obj};
Expand All @@ -731,7 +707,7 @@ struct old_cs
checksum_envelope<old_cs, serde::version<3>, serde::compat_version<2>> {
bool operator==(const old_cs&) const = default;

std::vector<test_msg1_new_manual> data_;
std::vector<test_msg1_new> data_;
auto serde_fields() { return std::tie(data_); }
};
struct new_no_cs
Expand All @@ -742,28 +718,28 @@ struct new_no_cs
}

serde::checksum_t unchecked_dummy_checksum_{0U};
std::vector<test_msg1_new_manual> data_;
std::vector<test_msg1_new> data_;

auto serde_fields() { return std::tie(unchecked_dummy_checksum_, data_); }
};

SEASTAR_THREAD_TEST_CASE(serde_checksum_update_1) {
const auto old_obj = old_cs{
.data_ = {
test_msg1_new_manual{
test_msg1_new{
._a = 1, ._m = test_msg0{._i = 33, ._j = 44}, ._b = 2, ._c = 3},
test_msg1_new_manual{
test_msg1_new{
._a = 4, ._m = test_msg0{._i = 55, ._j = 66}, ._b = 5, ._c = 6},
test_msg1_new_manual{
test_msg1_new{
._a = 7, ._m = test_msg0{._i = 77, ._j = 88}, ._b = 8, ._c = 9}}};

const auto new_obj = new_no_cs{
.data_ = {
test_msg1_new_manual{
test_msg1_new{
._a = 1, ._m = test_msg0{._i = 33, ._j = 44}, ._b = 2, ._c = 3},
test_msg1_new_manual{
test_msg1_new{
._a = 4, ._m = test_msg0{._i = 55, ._j = 66}, ._b = 5, ._c = 6},
test_msg1_new_manual{
test_msg1_new{
._a = 7, ._m = test_msg0{._i = 77, ._j = 88}, ._b = 8, ._c = 9}}};

const auto old_vec = std::vector<old_cs>{old_obj, old_obj};
Expand Down