serde: support variant #17537
serde: support variant #17537
Conversation
rockwotj
force-pushed
the
variant_serde
branch
3 times, most recently
from
609723f
to
255c264
Compare
|
I like the implementation; maybe this could accept only some sort of blessed variant, like (just an idea) a trait to specialize // header
namespace serde {
template<typename>
constexpr static auto is_blessed_variant = false;
}
// when declaring the variant
namespace serde{
template <>
constexpr static auto is_blessed_variant<std::variant<model::offset, model::revision_id>> = true;
}or a wrapper carrier type, like namespace serde{
template<typename ...Ts>
struct variant {
std::variant<Ts...> data;
};but both options carry some burden on the user |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47239#018ea0db-46b5-47ea-9f23-1fdd1fff9f10 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47382#018ea920-8a39-4257-a53a-c6dd6f417558 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47573#018ec3ef-e143-4aeb-8024-98784e990192 |
rockwotj
force-pushed
the
variant_serde
branch
2 times, most recently
from
fc98d8a
to
df56fbd
Compare
|
Force pushes: review feedback |
I created a transparent wrapper - I believe this works as expected, but would love a second pair of eyes, because this feels a bit like magic at this point... 馃槅 |
|
Force push: assert that std::variant doesn't work with serde but serde::variant does |
|
Force push: added more tests showing compatibility and addressing other review feedback. Thanks! |
|
Force push: fix |
rockwotj
force-pushed
the
variant_serde
branch
2 times, most recently
from
6fc59a6
to
a09b0ee
Compare
|
Force pushes: Review feedback about |
src/v/serde/test/serde_test.cc
Outdated
| }; | ||
|
|
||
| SEASTAR_THREAD_TEST_CASE(variant) { | ||
| using my_variant = serde::variant<ss::sstring, int, bool, my_enum, int>; |
There was a problem hiding this comment.
how about a test case that uses std::monostate?
There was a problem hiding this comment.
std::monostate is not supported by serde, should I add support for it?
|
ping here, the stacked PR for this is ready. I can either drop this and manually implement this pattern using custom serde or go forward with this approach. I am ambivalent |
I have an upcoming change to add a variant for a serde transforms struct, so I am in advance sending out this change to support variant in serde. Backwards compat: See the comment on serde::variant, but any additions/changes to the types in the variant are not backwards or forward compatible, so we need to handle this in some wrapper struct. This works by creating a compile time lookup table of variant index -> a lambda that creates the variant for the type at the given index. Then we do a runtime dispatch to the static function based on the index of the function (with a bounds check). The stuff you can do with templates are 馃く Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
|
Force pushes: more strict backwards compat |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/47573#018ec3ef-e147-4825-9d82-03192ed6b89b: |
I have an upcoming change to add a variant for a serde transforms
struct, so I am in advance sending out this change to support variant in
serde.
Variants are tricky w.r.t. compatibility, they are forward compatible if new
discriminants are appended to the variant.
Concretely that means that you can only read old versions of a variant if new
options are appended. Writing a new variant option should happen with a
feature barrier, so that an older reader will never read a newly written
variant.
This works by creating a compile time lookup table of variant index ->
a lambda that creates the variant for the type at the given index. Then
we do a runtime dispatch to the static function based on the index of
the function (with a bounds check).
The stuff you can do with templates are 馃く
Related: #17370
Backports Required
Release Notes