feat: expose snapshot load metrics in order to detect if log truncati…#11
feat: expose snapshot load metrics in order to detect if log truncati…#11
Conversation
|
|
||
| let (snapshot, load_metrics) = if let Some(limiter) = &config.log_size_limiter { | ||
| let original_segment = snapshot.log_segment().clone(); | ||
| let original_size: u64 = original_segment |
There was a problem hiding this comment.
@VLCDaniel can we do something like have a function in the newly added struct like from_segment_and_snapshot ?
I would try to move this code in the new struct. The reason is that is a new file, our code, so there will be less chance of merge conflicts if / when we upgrade to a new version
| (Arc::new(KernelSnapshot::new(truncated_segment, table_configuration)), metrics) | ||
| } else { | ||
| snapshot | ||
| let metrics = SnapshotLoadMetrics::from_snapshot(&snapshot); |
There was a problem hiding this comment.
@VLCDaniel this is great, see previous, we should have something like
let metric = SnapshotLoadMetrics::from_snapshot_and_segment...
| .try_into_arrow()?, | ||
| ); | ||
|
|
||
| // For updates, we don't track truncation metrics since we're building from existing snapshot |
There was a problem hiding this comment.
@VLCDaniel same comment here - could we have a line here and stuff this complexity inside SnapshotLoadMetrics ?
VLCDaniel
force-pushed
the
log-size-limiter-stats
branch
from
af39620 to
820526f
Compare
VLCDaniel
force-pushed
the
log-size-limiter-stats
branch
from
820526f to
0562010
Compare
| // At upgrade, RECHECK usage sites for DeltaTableConfig, we'll need to re-evaluate if | ||
| // stuff begins writing to it | ||
| let delta_table_config = DeltaTableConfig::default(); | ||
| let load_metrics = SnapshotLoadMetrics::from_snapshot(&exec_scan_plan_scan_snapshot); |
There was a problem hiding this comment.
nit: This is a little strange as the SnapshotLoadMetrics::from_snapshot could be hidden within snapshot (as it acts on the inner object ) - I think I don't like the fact that a SnapshotLoadMetrics comes from size_limits which really seems like an implementation detail
VLCDaniel
force-pushed
the
log-size-limiter-stats
branch
from
0562010 to
6b24db5
Compare
aditanase
left a comment
aditanase
left a comment
There was a problem hiding this comment.
much better integration into upstream codebase
4 participants
Fixed file size calculation, and added snapshot truncation metrics