serde: Require (serde_fields) XOR (explicit read and/or write)

[oleiman]

If a serde envelope implements either serde_read or serde_write, that envelope may not implement serde_fields.

PR also updates a couple of existing structs to account for this requirement.

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

[dotnwat]

yeh pretty much lgtm just a question or two

[dotnwat]

cc @rockwotj i think i'm still a bit confused about how exactly std::variant serde works, but on the surface this looks like a correct transformation from serde_fields to serde_read.

[oleiman]

tangential, but I wonder whether we need the bytes_left checks in here...

I was using topic_configuration::serde_read as an example, but column_store::serde_read is much more involved.

[dotnwat]

i don't think you need this part

   if (in.bytes_left() > h._bytes_left_limit) {
        in.skip(in.bytes_left() - h._bytes_left_limit);
    }

because it's done automatically in tag_invoke(read_tag) after serde_read is called. did you have a specific reason for adding it?

[dotnwat]

cc @oleiman

[oleiman]

done automatically inn tag_invoke

Ah, I misread that as embedded in the !has_serde_read branch.

specific reason for adding it

Nope. Saw it written that way somewhere and it stuck for whatever reason.

[rockwotj]

Don't we need a version check here? Only read this if version > 0, same for the version 2 fields only.

Or do these noop if there are no bytes left?

[felixguendling]

If called, read functions expect that there are bytes left to read. E.g. here:

redpanda/src/v/serde/rw/scalar.h

Lines 32 to 39 in 10e3f43

	if (unlikely(in.bytes_left() - bytes_left_limit < sizeof(Type))) {
	throw serde_exception(fmt_with_ctx(
	ssx::sformat,
	"reading type {} of size {}: {} bytes left",
	type_str<Type>(),
	sizeof(Type),
	in.bytes_left()));
	}

That's why the default version for reading an envelope is okay if the bytes_left were depleted and skips reading the following fields (compat_version vs serialized version comparison made sure this is okay):

redpanda/src/v/serde/rw/envelope.h

Lines 70 to 74 in 10e3f43

	envelope_for_each_field(t, [&](auto& f) {
	using FieldType = std::decay_t<decltype(f)>;
	if (h._bytes_left_limit == in.bytes_left()) {
	return false;
	}

[oleiman]

Ah, I completely misread this comment. Thanks @rockwotj

[dotnwat]

no rush on this, just wondering if i should be waiting on any more pushes?

[oleiman]

force push little cleanup

@dotnwat - I think the conclusion from these threads is that functionally we're good to go

[rockwotj]

No, transform options will fail to read old versions now, we need to fix that

[rockwotj]

Github is horrible at showing threads, but we need to resolve this: #23239 (comment)

See Felix's comment, old versions dont have all the fields so we need to explicitly check for the version before trying to read more fields

[dotnwat]

Github is horrible at showing threads, but we need to resolve this: #23239 (comment)

See Felix's comment, old versions dont have all the fields so we need to explicitly check for the version before trying to read more fields

Thanks for seeing the issue and blocking @rockwotj.

I'm a little bit unclear about what the concern is. My understanding is that @oleiman is replacing serde_fields for the read path, which as I understand it, wasn't doing any verison checks (because it was using the serde_fields interface). so in the replaced, explicit version of reads, those checks also aren't there.

i think either i'm misunderstanding how serde_fields is operating internally, how the replacement read calls work, or there was a bug in the original version?

[rockwotj]

wasn't doing any verison checks (because it was using the serde_fields interface). so in the replaced, explicit version of reads

serde_fields internally stops when there are no more bytes left, meaning you can not populate some fields (due to reading an old version that didn't write them).

We need that stopping early behavior either by checking if there are more bytes when reading fields for v1 or v2, or by explicitly doing version checks.

[oleiman]

Ah, forget all that. @rockwotj - You're talking about the versioning on transform_metadata, not the variant struct. Yeah, sorry for the confusion.

[oleiman]

force push version checks for model::transform_metadata

[oleiman]

CI Failures:

• log_segment_appender_test timed out on the bazel build
• debug build
  • CI Failure (key symptom) in NodesDecommissioningTest.test_recycle_all_nodes  #22717 (known, unrelated)
  • CI Failure (Data never drops below 1MB) in FullDiskReclaimTest.test_full_disk_triggers_gc #18014 (known, unrelated)

[vbotbuildovich]

new failures in https://buildkite.com/redpanda/redpanda/builds/54427#0191e7d4-c1fe-4d66-8085-2ea2ad4ee7ad:

"rptest.tests.nodes_decommissioning_test.NodesDecommissioningTest.test_recycle_all_nodes.auto_assign_node_id=False"

[vbotbuildovich]

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54427#0191e7d4-c1fe-4d66-8085-2ea2ad4ee7ad

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54427#0191e7d6-b110-429b-bc65-959f01f2682b