fix(operator): interpolate desc.url so failure message shows the URL#6800
fix(operator): interpolate desc.url so failure message shows the URL#6800kz930 wants to merge 4 commits into
Conversation
In URLFetcherOpExec, the fetch-failure fallback used `s"...$desc.url"`,
which expands only `$desc` (the URLFetcherOpDesc instance) and appends a
literal `.url`. Because LogicalOp overrides toString with
ToStringBuilder.reflectionToString, the "URL content" cell got the whole
descriptor dump (operatorId, inputPorts, dummyPropertyList, ...) instead
of the URL.
Wrap the member access in braces (`${desc.url}`) so the message reads
`Fetch failed for URL: <url>` as intended and stops leaking internal
operator fields into user-facing output.
Closes apache#6755
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6800 +/- ##
=========================================
Coverage 76.28% 76.28%
Complexity 3452 3452
=========================================
Files 1161 1161
Lines 45916 45915 -1
Branches 5099 5098 -1
=========================================
Hits 35025 35025
+ Misses 9327 9326 -1
Partials 1564 1564
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 384 | 0.234 | 25,917/32,533/32,533 us | 🔴 +21.5% / 🔴 +105.3% |
| 🟢 | bs=100 sw=10 sl=64 | 804 | 0.49 | 119,802/141,856/141,856 us | 🟢 -6.4% / 🔴 +33.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 914 | 0.558 | 1,099,532/1,122,693/1,122,693 us | ⚪ within ±5% / 🔴 +13.0% |
Baseline details
Latest main dda9e83 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 384 tuples/sec | 445 tuples/sec | 767.66 tuples/sec | -13.7% | -50.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.234 MB/s | 0.272 MB/s | 0.469 MB/s | -14.0% | -50.1% |
| bs=10 sw=10 sl=64 | p50 | 25,917 us | 21,323 us | 12,623 us | +21.5% | +105.3% |
| bs=10 sw=10 sl=64 | p95 | 32,533 us | 31,015 us | 16,011 us | +4.9% | +103.2% |
| bs=10 sw=10 sl=64 | p99 | 32,533 us | 31,015 us | 18,856 us | +4.9% | +72.5% |
| bs=100 sw=10 sl=64 | throughput | 804 tuples/sec | 823 tuples/sec | 998.44 tuples/sec | -2.3% | -19.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.49 MB/s | 0.502 MB/s | 0.609 MB/s | -2.4% | -19.6% |
| bs=100 sw=10 sl=64 | p50 | 119,802 us | 119,117 us | 100,463 us | +0.6% | +19.2% |
| bs=100 sw=10 sl=64 | p95 | 141,856 us | 151,609 us | 106,400 us | -6.4% | +33.3% |
| bs=100 sw=10 sl=64 | p99 | 141,856 us | 151,609 us | 117,712 us | -6.4% | +20.5% |
| bs=1000 sw=10 sl=64 | throughput | 914 tuples/sec | 914 tuples/sec | 1,034 tuples/sec | 0.0% | -11.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.558 MB/s | 0.558 MB/s | 0.631 MB/s | 0.0% | -11.6% |
| bs=1000 sw=10 sl=64 | p50 | 1,099,532 us | 1,097,911 us | 973,294 us | +0.1% | +13.0% |
| bs=1000 sw=10 sl=64 | p95 | 1,122,693 us | 1,164,412 us | 1,019,213 us | -3.6% | +10.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,122,693 us | 1,164,412 us | 1,049,896 us | -3.6% | +6.9% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,521.05,200,128000,384,0.234,25916.73,32533.39,32533.39
1,100,10,64,20,2488.84,2000,1280000,804,0.490,119802.42,141856.41,141856.41
2,1000,10,64,20,21877.23,20000,12800000,914,0.558,1099531.83,1122692.89,1122692.89|
@Yicong-Huang May you take a look at it please? |
Yicong-Huang
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix
Yicong-Huang
left a comment
There was a problem hiding this comment.
still would be good to add a regression test. if URLFetcher has no tests, let's add them in this PR.
…essage
Assert the fetch-failure fallback interpolates `desc.url` itself, not the
URLFetcherOpDesc reflectionToString dump. Uses a file:// URL to a nonexistent
path so getInputStreamFromURL returns None deterministically and offline (no
network), exercising the failure branch without external connectivity.
Verified: passes with the ${desc.url} fix, fails with the pre-fix $desc.url.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
891e204 to
86cd1c4
Compare
|
Done. Added regression test. |
There was a problem hiding this comment.
Pull request overview
Fixes a Scala string interpolation bug in the URL Fetcher source operator so the fetch-failure fallback message shows only the URL (and avoids leaking the full operator descriptor’s toString dump into user-facing output).
Changes:
- Fix string interpolation in
URLFetcherOpExecfailure-path message by interpolating${desc.url}. - Add a regression test that exercises the failure branch offline and asserts the message contains only the URL (and not descriptor fields).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/fetcher/URLFetcherOpExec.scala | Fixes failure message interpolation to use desc.url correctly. |
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/fetcher/URLFetcherOpExecSpec.scala | Adds regression coverage for the failure-path message content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kary Zheng <150742834+kz930@users.noreply.github.com>
|
Resolved Copilot comment. CI is green. |
|
thanks @kz930 . @aglinxinyuan codecov is not updating. please check. |
|
Dug in. The deferred codecov-action re-downloads and verifies the CLI on every leg, importing Codecov's key at runtime, and that key import flakes — a random leg dies while its siblings upload fine. With Fix in #6836: each upload retries once, so a transient CLI/GPG flake self-heals while a genuine failure still surfaces. Verification stays intact (no For #6800 specifically: its latest head is still working through the ASF runner backlog, so it should refresh once that Required Checks run + the deferred upload finish. If the upload flakes on it before #6836 lands, the quickest unblock is to re-run the failed |
What changes were proposed in this PR?
Fixes a Scala string-interpolation bug in
URLFetcherOpExec's fetch-failure fallback.The failure branch built its message with
s"Fetch failed for URL: $desc.url". In ans"..."interpolator,$descexpands only the identifierdesc(theURLFetcherOpDescinstance) and.urlis appended as literal text. BecauseLogicalOpoverridestoStringwithToStringBuilder.reflectionToString, the resultingURL contentcell contained the entire descriptor dump instead of the URL:This both malforms the message and leaks internal operator fields (
operatorId,inputPorts,dummyPropertyList, …) into user-facing output.The fix wraps the member access in braces so only
desc.urlis interpolated:Now the message reads as intended:
Any related issues, documentation, discussions?
Closes #6755
How was this PR tested?
Added an automated regression test to
URLFetcherOpExecSpecthat exercises the fetch-failure branch offline and deterministically: pointing the operator at afile://URL for a nonexistent path makesURLFetchUtil.getInputStreamFromURLreturnNonewith no network dependency. The test asserts the fallback cell is exactlyFetch failed for URL: <url>and does not contain the descriptor dump (URLFetcherOpDesc,operatorId, …), so it fails on the pre-fix$desc.urlbehavior and passes with the fix.Also reproduced end-to-end before the fix by pointing a URL Fetcher operator at an unreachable address (
https://this-host-does-not-exist.invalid/x) and inspecting theURL contentoutput cell: it showed the fullURLFetcherOpDesc[...]dump followed by.url; after the fix it showsFetch failed for URL: https://this-host-does-not-exist.invalid/x.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)