Skip to content
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

serde: Require T::serde_fields for all serde structs #23126

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented Aug 29, 2024

With the exception of those that implement BOTH a custom reader and a custom writer. In that case, the fields tuple is not needed, so envelope_to_tuple will not participate in overload resolution.

In effect, this PR modifies serde to require using serde_fields for all structs EXCEPT those implementing both serde_read and serde_write. In these cases envelope_to_tuple won't generally participate in overload resolution

Why do this? #21431 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

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.2.x
  • v24.1.x
  • v23.3.x

Release Notes

  • none

@oleiman oleiman self-assigned this Aug 29, 2024
@oleiman oleiman marked this pull request as ready for review September 5, 2024 17:49
@oleiman oleiman changed the title [DO NOT MERGE] serde: Require T::serde_fields for all serde structs serde: Require T::serde_fields for all serde structs Sep 5, 2024
@oleiman oleiman requested a review from dotnwat September 5, 2024 18:39
@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/54056#0191c394-ac5e-4c83-afcb-2e446caf341f:

"rptest.tests.e2e_shadow_indexing_test.EndToEndShadowIndexingTestWithDisruptions.test_write_with_node_failures.cloud_storage_type=CloudStorageType.S3"

@vbotbuildovich
Copy link
Collaborator

@oleiman
Copy link
Member Author

oleiman commented Sep 5, 2024

CI Failure:

dotnwat
dotnwat previously approved these changes Sep 5, 2024
With the exception of those that implement BOTH a custom reader
and a custom writer. In that case, the fields tuple is not needed,
so envelope_to_tuple will not participate in overload resolution.

Also removes has_serde_fields concept, which is no longer needed.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman
Copy link
Member Author

oleiman commented Sep 5, 2024

force push to remove has_serde_fields concept.

@oleiman
Copy link
Member Author

oleiman commented Sep 5, 2024

bazel build failed initially - looks to have been a transient 502 error pulling something from github

@oleiman
Copy link
Member Author

oleiman commented Sep 6, 2024

CI Failure:

  • cluster_cloud_metadata_rpfixture timed out build

@oleiman oleiman merged commit 1ad0a0e into redpanda-data:dev Sep 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants