c/st: self_test_result rpc: move {start,end}_time to end of layout #21431
Conversation
|
My read of redpanda/src/v/serde/rw/envelope.h Lines 66 to 81 in b33bd06 is that the read end sticks the deser'ed value into an instance of the envelope via a reference to the corresponding field (and does this in the order returned by |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51593#0190bcf2-27a2-4c06-96f8-91b3026c3800 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51593#0190c1ec-fe5c-4294-b623-f0deed21e0cf ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51744#0190c7fc-0ba9-466b-a5bd-e689f5412134 |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/51593#0190c1ec-fe5c-4294-b623-f0deed21e0cf: |
|
CI Failures:
|
|
/ci-repeat 1 |
Effectively turning the original change into an append and obviating the need for a compat version bump. It's not immediately clear why the struct was laid out this way in the first place. A reasonable guess is some balance of: - The original ordering makes good sense for the aggregate in-memory - It's an rpc format and not intended for on-disk persistence Still, the compat version change means that if a self test goes off during an upgrade across the compat boundary, that rpc will fail. Adjusting the layout via envelope::serde_fields means that we can maintain the nice field order semantics in the aggregate while ensuring that the diff looks like a field append from serde's view.
oleiman
force-pushed
the
etc/noticket/self-test-result-layout
branch
from
7b992c1 to
3c38672
Compare
|
force push empty |
|
Tests seem to have succeeded here... infra failure? https://buildkite.com/redpanda/redpanda/builds/51744#0190c7fd-83ef-4db7-ac59-a5bbb44a8f41 |
Definitely seems that way... the tests all passed it looks like? |
Yes, it looks that way to me. Report looks good anyway. Not immediately clear what failed, but it wasn't in ducktape. |
| auto serde_fields() { | ||
| return std::tie( | ||
| p50, | ||
| p90, | ||
| p99, | ||
| p999, | ||
| max, | ||
| rps, | ||
| bps, | ||
| timeouts, | ||
| test_id, | ||
| name, | ||
| info, | ||
| test_type, | ||
| duration, | ||
| warning, | ||
| error, | ||
| start_time, | ||
| end_time); | ||
| } |
There was a problem hiding this comment.
nice. more broadly, i kinda think we should change serde to require using serde_fields instead of the implicit ordering in the struct. wdyt?
There was a problem hiding this comment.
Yeah agreed. I don't really get the need for it generally; besides which i believe it maxes out at 20 fields or something like that. Pretty marginal as a convenience feature IMO.
There was a problem hiding this comment.
I'd be happy to knock that out soon. See what people think
Effectively turning the original change into an append and obviating the need for a compat version bump.
It's not immediately clear why the struct was laid out this way in the first place. A reasonable guess is some balance of:
Still, the compat version change means that if a self test goes off during an upgrade across the compat boundary, that rpc will fail. Adjusting the layout via envelope::serde_fields means that we can maintain the nice field order semantics in the aggregate while ensuring that the diff looks like a field append from serde's view.
Backports Required
Release Notes