persist: add flag for lower bound validations on read #32802
Conversation
ptravers
changed the title
src/txn-wal/src/operator.rs
Outdated
| let data_stream = data_stream.flat_map(|part| { | ||
| let part = part.parse(); | ||
| let data_stream = data_stream.flat_map(move |part| { | ||
| let part = part.parse(persist_cfg.clone()); |
There was a problem hiding this comment.
this is all pretty gross. I wonder if it would make more sense to have the public parse just take a bool for validation enabled?
There was a problem hiding this comment.
I think the least gross version would be to store the FetchConfig as a field in the BatchFetcherConfig, then add it as a field on FetchedBlob. That should prevent any changes outside the persist-client codebase, and it's lightly more consistent...
| @@ -369,6 +370,7 @@ pub async fn blob_batch_part( | |||
| } | |||
|
|
|||
| async fn consolidated_size(args: &StateArgs) -> Result<(), anyhow::Error> { | |||
| let cfg = PersistConfig::new_default_configs(&READ_ALL_BUILD_INFO, SYSTEM_TIME.clone()); | |||
There was a problem hiding this comment.
I'd use state_versions.cfg instead here if we can, just to avoid having to potentially-divergent configs floating around...
src/persist-client/src/fetch.rs
Outdated
| Self { | ||
| validate_lower_bounds_on_read: cfg.fetch_validate_lower_bounds_on_read, | ||
| } | ||
| } |
There was a problem hiding this comment.
If you don't feel strongly, can we store the FetchConfig as a field on CompactConfig, like we do for the BatchBuilderConfig? A bit closer to the existing code / saves some duplication.
src/txn-wal/src/operator.rs
Outdated
| let data_stream = data_stream.flat_map(|part| { | ||
| let part = part.parse(); | ||
| let data_stream = data_stream.flat_map(move |part| { | ||
| let part = part.parse(persist_cfg.clone()); |
There was a problem hiding this comment.
I think the least gross version would be to store the FetchConfig as a field in the BatchFetcherConfig, then add it as a field on FetchedBlob. That should prevent any changes outside the persist-client codebase, and it's lightly more consistent...
Motivation
9399
Tips for reviewer
adds a feature flag to enable turning off validation of batch vs part lower bounds on read from persist.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.