offline_log_viewer: allow reading newer versions of envelopes #23803
Conversation
nvartolomei
force-pushed
the
nv/offline-log-viewer-forward-compatibility
branch
from
f4cd3ec
to
612e962
Compare
|
/dt |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/56669#01929767-0169-4b36-bfa4-2faaa8852570 |
Retry command for Build#56669please wait until all jobs are finished before running the slash command |
|
Only one known failure |
| rdr.read_envelope(read_topic_properties_serde, max_version=10), | ||
| rdr.read_envelope(read_topic_properties_serde, max_compat_version=10), |
There was a problem hiding this comment.
first contact w/ this code, so sorry if these are obvious. few q's out of curiosity:
- the max_version/max_compat_version here is expected to always be in sync w/
T::redpanda_serde_versionright? along w/ the correspondingtype_readthing?
a. thought: couldn't we keep the max version & field reader info together? or is that not general enough? - All this is updated manually, ideally in lockstep with changes to relevant envelopes?
- Assuming (2), have we ever talked about auto-generating a table or something? Thinking about it for 2s it sounds really complicated, but idk.
a. 😈 idea: Python bindings forv::serde
tools/offline_log_viewer/reader.py
Outdated
| if envelope.compat_version <= max_compat_version: | ||
| v = type_read(self, envelope.version) | ||
|
|
||
| # Skip the rest of the envelope. | ||
| bytes_left_in_envelope -= self.stream.tell( | ||
| ) - envelope_start_pos | ||
| self.stream.read(bytes_left_in_envelope) |
There was a problem hiding this comment.
Do we need to cover this case:
redpanda/src/v/serde/read_header.h
Line 75 in 3f944f5
Like if the type_read implies a compat version that exceeds the encoded version of the binary blob we're reading, should we reject that as well?
nvartolomei
force-pushed
the
nv/offline-log-viewer-forward-compatibility
branch
from
612e962
to
2e45890
Compare
Before this commit offline log viewer would refuse reading envelopes if their version was higher than max_version. That is an overly restrictive constraint. Our envelopes are designed to be forward and backwards compatible so in most of the cases an older implementation of the log viewer should be able to read newer versions of the envelopes just fine. This commit implements that. Instead of putting a constraint on version we constrain only compat_version like regular redpanda deserialization would do. Additional changes include issuing a warning if offline log viewer compat version is lower than envelope version. This means that offline log viewer will be able to decode only a subset of the envelope. And obviously added code to skip over data that can't be read by the offline log viewer. Breaking changes in the envelopes are introduced by bumping compat version. In that case offline log viewer will refuse the decoding as expected.
|
@oleiman great feedback. I forgot to consider when serde structs bump compat_version. Force-pushed some changes, the meat of the change is the highlighted lines What do you think about merging this and working on compat_version thing later? I did spot check and couldn't find any structs relevant for offline log viewer that have compat_version > 0. My thinking is that offline log viewer should be allowed to evolve separately from the rest of the codebase and a newer log viewer should always be able to read older data. This is what this commit achieves. The only exception at this moment is when compat_version is bumped. To support reading older logs after that point we will need to provide multiple read_type implementations somehow. One for each compat_version. |
There was a problem hiding this comment.
What do you think about merging this and working on compat_version thing later? I did spot check and couldn't find any structs relevant for offline log viewer that have compat_version > 0.
sgtm
My thinking is that offline log viewer should be allowed to evolve separately from the rest of the codebase and a newer log viewer should always be able to read older data. This is what this commit achieves.
The only exception at this moment is when compat_version is bumped. To support reading older logs after that point we will need to provide multiple read_type implementations somehow. One for each compat_version.
Nice, makes sense. There's probably a mostly mechanical refactor lurking there somewhere; good to know it's not critical based on today's serde landscape. Thanks for the explanation 🙂
Before this commit offline log viewer would refuse reading envelopes if their version was higher than max_version. That is an overly restrictive constraint. Our envelopes are designed to be forward and backwards compatible so in most of the cases an older implementation of the log viewer should be able to read newer versions of the envelopes just fine.
This commit implements that. Instead of putting a constraint on version we constrain only compat_version like regular redpanda deserialization would do.
Additional changes include issuing a warning if offline log viewer compat version is lower than envelope version. This means that offline log viewer will be able to decode only a subset of the envelope.
And obviously added code to skip over data that can't be read by the offline log viewer.
Breaking changes in the envelopes are introduced by bumping compat version. In that case offline log viewer will refuse the decoding as expected.
Backports Required
Release Notes