use option to replace expect with context and avoid creating two gap detectors #428
Conversation
| @@ -132,24 +144,20 @@ pub async fn create_gap_detector_status_tracker_loop( | |||
| // We don't panic as everything downstream will panic if it doesn't work/receive | |||
| } | |||
|
|
|||
| if let Some(res_last_success_batch) = res.last_success_batch { | |||
There was a problem hiding this comment.
Why do we need to remove this?
There was a problem hiding this comment.
I've decided to remove the last_success_batch field from the DefaultGapDetectorResult due to the complexity and limited utility it offered in tracking progress within the parquet processor. Given the processor's architecture, where 10 tasks each manage a buffer containing distinct transaction ranges, the end_version of a batch isn't necessarily sequential. This non-sequential nature made it challenging to effectively use end_version and last_transaction_timestamp for status updates.
Instead, I'm proposing that we update the processor status using next_version_process - 1. This approach aligns better with the purpose of the status ensuring data integrity.
But as I am writing this, we are running 10 tasks only for backfilling, but I tuned to 1 task for regular traffic, which doesn't apply to the case above. let me come back with a better one that is applicable to both cases.
There was a problem hiding this comment.
I thought about it, maybe keep this behaviorfor now since backfilling is our priority, and then improve later to support both cases as a follow-up?
There was a problem hiding this comment.
I'm bouncing back and forth between the new and old code and can't quite figure out what changed. At least here it seems like we're still logging the same stuff as before, but more reliably now, since the fields are always set. With the new code the structs seem less deep / cleaner. tldr I need to read the code further to figure out what's going on here, but don't let it block you, I'll catch up later.
yuunlimm
force-pushed
the
yuunlimm/parquet-gap-detector-fix
branch
4 times, most recently
from
39b295a
to
0488e8f
Compare
rust/processor/src/db/common/models/default_models/parquet_write_set_changes.rs
Outdated
Show resolved
Hide resolved
| @@ -132,24 +144,20 @@ pub async fn create_gap_detector_status_tracker_loop( | |||
| // We don't panic as everything downstream will panic if it doesn't work/receive | |||
| } | |||
|
|
|||
| if let Some(res_last_success_batch) = res.last_success_batch { | |||
There was a problem hiding this comment.
I'm bouncing back and forth between the new and old code and can't quite figure out what changed. At least here it seems like we're still logging the same stuff as before, but more reliably now, since the fields are always set. With the new code the structs seem less deep / cleaner. tldr I need to read the code further to figure out what's going on here, but don't let it block you, I'll catch up later.
yuunlimm
force-pushed
the
yuunlimm/parquet-gap-detector-fix
branch
from
86cfcb9
to
171609e
Compare
…d encoding beforehand to call this function. - reduced the duplicated lines of code - Use context instead of unwraping with unsafe assumptions - make gap detector trait fn to return Result type without requiring a unnecessary error type
yuunlimm
force-pushed
the
yuunlimm/parquet-gap-detector-fix
branch
from
c1f9e04
to
5f3d48a
Compare
yuunlimm
force-pushed
the
yuunlimm/parquet-gap-detector-fix
branch
from
5f3d48a
to
e4593aa
Compare
Summary
Test Plan
Tested locally and verified status being updated
