Repository navigation
Add durability to Databricks Lakeflow - #1223
Conversation
Signed-off-by: yaron2 <schneider.yaron@live.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1223 +/- ##
==========================================
+ Coverage 83.93% 84.30% +0.37%
==========================================
Files 123 132 +9
Lines 10271 10534 +263
==========================================
+ Hits 8621 8881 +260
- Misses 1650 1653 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CasperGN
left a comment
There was a problem hiding this comment.
Thanks, this is a useful integration, and the delivery-semantics write-up is clear. The check-then-schedule flow in scheduling.py and the generation epoch for full refreshes both look right to me. tests/ext/databricks passes (71). A few things before this merges:
Blocking
-
Composite keys can collide, and the second record is silently dropped.
id_fieldsvalues are joined with_(identity.py L120), so('x_y', 'z')and('x', 'y_z')both produceorders-s-v1-x_y_z. The second record then finds the first record's instance and is logged asalready_existed, so its workflow never runs. Please encode the components unambiguously before sanitizing, for example a JSON array or length-prefixed parts, and add a test with two keys that differ only in where the separator falls. -
The fallback identity depends on row order. With no key configured, the ID is
<batch_id>-<record_index>(identity.py L169). Spark doesn't guarantee the same row order when a micro-batch is retried, so a retry can give record B the index record A had. B is then skipped as already handled, and A is scheduled again under B's old index. Please either require one ofid_field/id_fields/instance_id_factory, or make position-based identity an explicit opt-in whose docs say it's only safe for deterministically ordered sources. -
The whole batch is buffered in memory despite the docstring.
processsubmits every row to the executor as fast as the iterator yields them (batch_handler.py L122-L135).ThreadPoolExecutor's queue is unbounded, somax_in_flightlimits concurrency but not memory. Withmax_in_flight=2and a slow sidecar, all 2000 rows of a test batch were already pulled offtoLocalIterator()before the first workflow was scheduled. That contradicts "a micro-batch never has to fit entirely in driver memory at once" (L96). A semaphore aroundsubmit(released in a done-callback) would bound it tomax_in_flightrows.
Non-blocking
id_fields='order_id'(a bare string) is accepted and treated as the fieldso,r,d, … (config.py L65-L66). Rejectingstrthere would catch an easy mistake.max_records_per_batchfails after scheduling up to the limit (L125). Every retry repeats the same partial work and fails again, so the pipeline stays stuck until the config changes. That's fine as a design choice, but worth a sentence in the docs.toLocalIteratorfallback. On Spark Connect,toLocalIteratoris a generator, so an "unsupported" error would surface on the firstnext(), not at the call inside thetry(L165). If the serverless error you observed really is raised at the call, a short comment saying so would help. Otherwise, moving the firstnext()inside the guard makes the fallback work on both.
CasperGN
left a comment
There was a problem hiding this comment.
Thanks! Re-reviewed at 142752b. All six points from the earlier pass are addressed, with tests:
- Composite keys are JSON-encoded (
identity.py) instead of_-joined, which closes the collision. Regression test added. - The position-based fallback identity now requires an explicit
allow_batch_position_identity=True. By default, config construction fails. process()bounds read-ahead with athreading.Semaphore(max_in_flight)released in a done-callback, andMemoryBoundedIterationTestscovers it.- A bare-string
id_fieldsis rejected. - The Spark Connect lazy
toLocalIterator()failure is covered, because the first row is now pulled inside the guard. - The docs now say that
max_records_per_batchfails the same way on every retry.
tests/ext/databricks passes (78), and mypy/ruff format are clean. Two minor notes for a follow-up, not this PR: MemoryBoundedIterationTests depends on timing (the slack is generous), and allow_batch_position_identity=True does nothing when a real key strategy is also set.
This PR adds an extension that turns Databricks Lakeflow data into durable business actions without building your own retry-safe orchestration layer.
dapr.ext.databricks fixes this with one function call:
re-triggered.