Conversation
56f4f84 to
52065ab
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Three hard blocks, one correctness concern.
mythical-fred
left a comment
There was a problem hiding this comment.
Two code bugs (inline) plus missing docs — blocking on the docs.
threads and fatal_on_max_retries are new user-visible config options but neither appears in docs.feldera.com/docs/connectors/sinks/delta.md. The Documentation checkbox is unchecked. Please add entries to the config table for both options.
6738817 to
ac1dfbd
Compare
ac1dfbd to
adf3e2c
Compare
ryzhyk
left a comment
There was a problem hiding this comment.
Since we don't do anything meaningful with fatal errors today, let's not bother adding that option. We'll do another pass to rework connector error handling later.
| } | ||
|
|
||
| // prints timing for manual comparison, and asserts output correctness | ||
| #[test] |
There was a problem hiding this comment.
Let's turn this into a proper benchmark under adapters/bench.
What benchmark results did you get?
There was a problem hiding this comment.
$cargo bench --package dbsp_adapters --bench delta_encoder --features with-deltalake
Running benches/delta_encoder.rs (target/release/deps/delta_encoder-836871c7c7c0c9cd)
Gnuplot not found, using plotters backend
delta_indexed_encode/workers/1
time: [35.170 ms 35.201 ms 35.233 ms]
thrpt: [2.8382 Melem/s 2.8408 Melem/s 2.8433 Melem/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
delta_indexed_encode/workers/2
time: [24.756 ms 25.310 ms 25.873 ms]
thrpt: [3.8650 Melem/s 3.9511 Melem/s 4.0394 Melem/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
delta_indexed_encode/workers/4
time: [15.615 ms 15.899 ms 16.199 ms]
thrpt: [6.1734 Melem/s 6.2897 Melem/s 6.4042 Melem/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild
delta_indexed_encode/workers/8
time: [9.1359 ms 9.2460 ms 9.3869 ms]
thrpt: [10.653 Melem/s 10.816 Melem/s 10.946 Melem/s]
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severe
mythical-fred
left a comment
There was a problem hiding this comment.
My previous two blockers (schema minimum = 1 and docs for threads) are resolved — thank you. One remaining issue:
ryzhyk
left a comment
There was a problem hiding this comment.
Thanks for addressing all comments! I only have a couple of questions left (inline)
47b7efc to
74d37da
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
My previous blockers are resolved: schema validation, docs, and commit message are all fixed. Upgrading to approve.
74d37da to
c7172d7
Compare
Use SplitCursor to split the batch and distribute it across tasks, each task retries encoding and writing to delta lake and then returns Add actions which main task retries to commit to delta lake Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
c7172d7 to
b29e7b7
Compare
Describe Manual Test Plan
Added unit tests and ran locally
for write retry, I changed local permissions for local path to simulate write failure
and to restore
& commit retries were tried by renaming the directory
what can be tested further:
Checklist
Breaking Changes?
we're adding new config options for delta connector but should not be a breaking change.