-
Notifications
You must be signed in to change notification settings - Fork 592
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
Require serde_fields for all serde structs #22782
Require serde_fields for all serde structs #22782
Conversation
/ci-repeat 1 |
271c030
to
9129656
Compare
/ci-repeat 1 |
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6009-4b03-aec1-ed4996c0a79a:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6004-42f7-9508-1976d10b810b:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6006-4028-9934-40514b596708:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6007-482b-a8bc-53eb021338c9:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f60-be5f-4e6c-a136-374e8e806250:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f60-be5b-41f7-8d04-7012a56d0c28:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f60-be61-40f3-ba20-106bfde29737:
new failures in https://buildkite.com/redpanda/redpanda/builds/52635#01912f60-be5d-4e61-ab1b-2ff1bf9730d0:
new failures in https://buildkite.com/redpanda/redpanda/builds/52977#0191550a-eb87-4aad-8a64-26539e006c3b:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#0191773e-a962-47fa-9b1e-dc702a9434ca:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#0191773e-a964-490d-a17c-8ae15bc6540d:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#0191773e-a966-4924-9ae6-4786ca3455f2:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#0191773e-a968-4cf0-aa45-90e910853caf:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#01917740-42b8-4b8a-a8b2-f49bc0c88379:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#01917740-42b9-4778-9f6c-14e23bce22e6:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#01917740-42b4-47ec-b55d-ccc8baf8d67d:
new failures in https://buildkite.com/redpanda/redpanda/builds/53329#01917740-42b6-4ab8-b9c1-f9a75988966e:
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6004-42f7-9508-1976d10b810b ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52635#01912f5f-6007-482b-a8bc-53eb021338c9 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52635#01912f60-be5b-41f7-8d04-7012a56d0c28 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52780#01914326-9a3d-4dd4-91df-5f62e843d081 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/53329#0191773e-a966-4924-9ae6-4786ca3455f2 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/53329#01917740-42b6-4ab8-b9c1-f9a75988966e |
9129656
to
2647521
Compare
/ci-repeat 1 |
2647521
to
ec8276a
Compare
/ci-repeat 1 |
ec8276a
to
7070fc6
Compare
/ci-repeat 1 |
CI Failure:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should go further and require serde_fields except for custom implementations of serde_read and serde_write. so even for single field structs?
Ha, yeah, I've been avoiding flipping this to ready for exactly that reason. Leaning towards yes; better to require it up front vs. spewing template mess on the next append. |
This is awesome, +1. |
7070fc6
to
cc40bc4
Compare
/ci-repeat 1 |
cc40bc4
to
2a1904c
Compare
/ci-repeat 1 |
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
- archival_metadata_stm::segment - archival_metadata_stm::start_offset_with_delta - archival_metadata_stm::truncate_archive_commit_cmd Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
2a1904c
to
b190447
Compare
force push to rebase dev and fix a merge conflict |
/ci-repeat 1 |
CI Failures:
|
b190447
to
1c0ee0f
Compare
force push to fix an integration issue and a couple omissions. |
@@ -29,177 +29,6 @@ constexpr inline auto envelope_to_tuple(T&& t) { | |||
return t.serde_fields(); | |||
} | |||
|
|||
template<typename T> | |||
requires(!detail::has_serde_fields<T>) | |||
constexpr inline auto envelope_to_tuple(T& t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💣
@felixguendling sup? |
The general idea LGTM. Make sure to check that all necessary fields are (de)serialized when doing a code review (before this check was not necessary with "automatic mode" aka aggregate to tuple). |
CI Failure:
|
Yeah, previously flakey, there was a revert PR that should fix this that got merged this AM. Re-running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k, i've been through this line by line, and it looks clean modulo some commit messages that have drifted out of date. @dotnwat anyone else i should tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit msg is out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit msg out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit msg is out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably fold into previous commit
@oleiman do you think we should add a static assert if it doesn't already exist that serde_fields xor serde_read/write are used so we don't have to think about ambiguity if both are present? |
Seems like a good idea. I think in some cases we use serde_fields on the write path with explicit reader for compat reasons, but that stuff can be rejiggered as well. Anyway, I'm going to throw this PR into draft mode and split it up. Thanks @dotnwat & @WillemKauf for feedback 🙂 |
superseded by #23126 et al. |
In effect, this PR modifies serde to require using
serde_fields
for all structs EXCEPT those implementing bothserde_read
andserde_write
. In these casesenvelope_to_tuple
won't generally participate in overload resolutionAlso adds
serde_fields()
to several existing structs, including those generated automatically for fuzz-testing.Why do this? This PR provides a motivating example. Where possible, we try to avoid compat-breaking serde changes by appending new fields to the end of the binary format. The implicit coupling between aggregate field ordering and binary format makes it easy to introduce subtle serialization bugs or unnecessary compat breaks.
Backports Required
Release Notes