Serde: do not use reflection #29156
Serde: do not use reflection #29156
Conversation
To remove serde dependency on refelction in future
There is support for classes that can be written/read by serde, but do not depend on serde::envelope. These still need version and compat_version static members, but when read/written in automatic mode instead of serde_field() rely on reflection. Remove such support.
There was a problem hiding this comment.
Pull request overview
This PR removes support for serde classes that don't inherit from serde::envelope but instead use manual version declarations with reflection-based serialization. The change simplifies the serde implementation by requiring all serializable types to properly inherit from serde::envelope.
Key changes:
- Removed the
inherits_from_envelopeconcept and related reflection-based serialization paths - Converted manual serde structs (
segment_meta,segment_meta_v0,segment_meta_v1) to properly inherit fromserde::envelope - Updated the
is_envelopeconcept to check for inheritance rather than just version member presence
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/serde/envelope.h | Simplified is_envelope concept to check inheritance flag; removed inherits_from_envelope concept |
| src/v/serde/envelope_for_each_field.h | Removed reflection-based code paths and dependencies |
| src/v/serde/BUILD | Removed dependencies on reflection utilities (arity, to_tuple) |
| src/v/serde/test/serde_test.cc | Removed test struct test_msg1_new_manual and related tests; updated references to use envelope-based types |
| src/v/serde/test/fuzz.cc | Replaced reflection-based arity calculation with tuple size from serde_fields() |
| src/v/cloud_storage/types.h | Converted segment_meta to inherit from serde::envelope and added serde_fields() method |
| src/v/cloud_storage/tests/partition_manifest_test.cc | Converted segment_meta_v0 and segment_meta_v1 to inherit from serde::envelope |
| src/v/cloud_storage/segment_meta_cstore.cc | Updated arity check to use tuple size from serde_fields() instead of reflection |
CI test resultstest results on build#78592
|
| std::apply( | ||
| [&](auto&&... args) { (fn(args), ...); }, reflection::to_tuple(t)); | ||
| } | ||
| std::apply([&](auto&&... args) { (fn(args), ...); }, envelope_to_tuple(t)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
4 participants
Currently serde supports classes not based on serde::envelope. These still need version and compat_version static members, but serde reads/writes them in automatic mode, instead of serde_field() it uses reflection. Remove such classes and support for them.
Backports Required
Release Notes