You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/08/15 08:26:00 UTC
[PR] test(amber): cover the reconfiguration service past its own test seams [texera]
aglinxinyuan opened a new pull request, #7692:
URL: https://github.com/apache/texera/pull/7692
### What changes were proposed in this PR?
`ExecutionReconfigurationService` was at **51.4% of 35 lines**, and the cause was structural: the existing spec's `RecordingService` passed `client = null` / `workflow = null` and overrode all three protected seams, so the uncovered residue was exactly what those overrides skip. In particular, any test that keeps the `dispatch` override leaves line 110 dark however much it asserts.
Tests **10 -> 13**. Lines **18/35 (51.4%) -> 35/35 (100%)**, branches 5/6, measured with only this spec running.
The spec now builds a real empty-plan `AmberClient` with a mocked `coordinatorInterface` (the pattern at `ExecutionRuntimeServiceSpec.scala:71-95`) and leaves the seams alone, so `modifyOperatorLogic`, the dispatch body, the worker-completion callback and the completion diff handler all execute for real.
### Verification
Review proposed nine production mutations. **All nine survived the 10-test suite.** Eight are now killed, each verified red on a named test from the JUnit XML, one mutation at a time with an asserted-empty production diff between runs. The ninth is deliberately left alive, for a reason worth spelling out below.
| Surviving mutation | Why nothing noticed |
|---|---|
| `old.unscheduledReconfigurations :+ op` -> `List(op)` | only one edit was ever made, so append and overwrite were indistinguishable |
| the failure arm wiping the queue | no pending work existed when the rejection was tested |
| `runtimeReconfiguration(..., currentOp, newOp)` -> `(..., newOp, newOp)` | nothing recorded which descriptor the service treats as "old" |
| dropping the `StateTransferFunc` | the queue assertion discarded the tuple's second element |
| carrying `completedReconfigurations` across a new round | no stale worker was ever seeded |
| deleting the guard's second leg | no test straddled a round boundary |
| announce-once-per-operator, and `(new -- old).take(1)` | every diff carried exactly one worker |
The fixes strengthen existing tests rather than bolting on new ones where possible: the queue test now makes two edits and checks order, per-slot limit and the state-transfer closure; the rejection test seeds pending work; the dispatch test seeds a stale completed worker. Three tests are new, including two late-subscriber cases that exercise `StateStore`'s catch-up replay pair -- one with a two-worker diff, one straddling a round boundary.
### Two mutations are deliberately left alive
**The `&&` -> `||` flip at line 138** is not killable through the service's own API. It only changes the outcome for a transition that atomically retires a round *while* adding a completed worker, and production has exactly three transitions on this store, none of which can produce that state. The single-leg flip of the same condition **is** killed, so the line is pinned; only the connective is not.
**Wrapping the discarded `Disposable` at line 117 in `addSubscription`** survives by design. That is the shape of the *production fix* for a leak (see below), and pinning it either way would mean asserting the leak. The suite was instead made agnostic to how it gets resolved.
### Deliberately not included
Line 142's `workflow.physicalPlan` dereference is covered but **not pinnable**: `getPhysicalOpByWorkerId` looks the operator up by the name parsed out of the worker id, so every observable value is identical to a plan-free implementation. The only difference is that the real code throws for a worker absent from the plan -- asserting that would cement a crash, not a contract.
Two defects are reported rather than pinned. Line 117 discards the `Disposable` returned by the engine callback registration, so the callback outlives the service. And `ReconfigurationHandler.scala:138-139` attaches `notifyOnComplete` per **worker**, while this file's comment describes per-operator announcement -- so an operator with several workers is announced more than once. The announcement test is renamed to the contract-neutral "announce the logical operator of every worker that reports in" and a second worker is added to the fixture, rather than asserting a once-per-operator contract that current production does not honour.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7691
### How was this PR tested?
```
STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.ExecutionReconfigurationServiceSpec"
```
```
[info] Total number of tests run: 13
[info] Tests: succeeded 13, failed 0, canceled 0, ignored 0, pending 0
```
The spec's base becomes `TestKit(ActorSystem(...))` with `AnyFlatSpecLike` so the empty-plan client can be built; the system name is suite-unique and shut down in `afterAll`, as two sibling specs in the same package already do. `Test/scalafmtCheck` and `Test/scalafix --check` both pass.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(amber): cover the reconfiguration service past its own test seams [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #7692:
URL: https://github.com/apache/texera/pull/7692
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(amber): cover the reconfiguration service past its own test seams [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7692:
URL: https://github.com/apache/texera/pull/7692#issuecomment-5301370247
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@Yicong-Huang`
You can notify them by mentioning `@Yicong-Huang` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(amber): cover the reconfiguration service past its own test seams [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7692:
URL: https://github.com/apache/texera/pull/7692#issuecomment-5301411642
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7692?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 90.67%. Comparing base ([`1022c7d`](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8426335`](https://app.codecov.io/gh/apache/texera/commit/84263358b887aa56f07f2d6a1d619ca01c8d8315?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #7692 +/- ##
============================================
+ Coverage 90.64% 90.67% +0.03%
- Complexity 4439 4443 +4
============================================
Files 1175 1175
Lines 47140 47140
Branches 5284 5284
============================================
+ Hits 42729 42746 +17
+ Misses 2697 2679 -18
- Partials 1714 1715 +1
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.62% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.36% <ø> (+0.09%)` | :arrow_up: | |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.46% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.55% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.90% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `92.15% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.31% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.52% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7692/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.89% <ø> (ø)` | | Carriedforward from [1022c7d](https://app.codecov.io/gh/apache/texera/commit/1022c7de9dc4cb0d63073b5c2686918597b55398?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7692?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(amber): cover the reconfiguration service past its own test seams [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7692:
URL: https://github.com/apache/texera/pull/7692#issuecomment-5301392166
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 7 worse · ⚪ 8 noise (<±5%) · 0 without baseline
> Compared against main `1022c7d` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/31874467917)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 374 | 0.228 | 25,608/35,063/35,063 us | 🔴 +12.2% / 🔴 +114.5% |
| 🔴 | bs=100 sw=10 sl=64 | 776 | 0.474 | 125,227/164,273/164,273 us | 🔴 +9.3% / 🔴 +54.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 905 | 0.553 | 1,110,934/1,139,151/1,139,151 us | ⚪ within ±5% / 🔴 +13.8% |
<details><summary>Baseline details</summary>
Latest main `1022c7d` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 374 tuples/sec | 420 tuples/sec | 779.28 tuples/sec | -11.0% | -52.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.228 MB/s | 0.257 MB/s | 0.476 MB/s | -11.3% | -52.1% |
| bs=10 sw=10 sl=64 | p50 | 25,608 us | 22,833 us | 12,712 us | +12.2% | +101.4% |
| bs=10 sw=10 sl=64 | p95 | 35,063 us | 32,930 us | 16,345 us | +6.5% | +114.5% |
| bs=10 sw=10 sl=64 | p99 | 35,063 us | 32,930 us | 19,050 us | +6.5% | +84.1% |
| bs=100 sw=10 sl=64 | throughput | 776 tuples/sec | 805 tuples/sec | 1,017 tuples/sec | -3.6% | -23.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.474 MB/s | 0.491 MB/s | 0.621 MB/s | -3.5% | -23.7% |
| bs=100 sw=10 sl=64 | p50 | 125,227 us | 121,973 us | 100,048 us | +2.7% | +25.2% |
| bs=100 sw=10 sl=64 | p95 | 164,273 us | 150,333 us | 106,477 us | +9.3% | +54.3% |
| bs=100 sw=10 sl=64 | p99 | 164,273 us | 150,333 us | 114,739 us | +9.3% | +43.2% |
| bs=1000 sw=10 sl=64 | throughput | 905 tuples/sec | 908 tuples/sec | 1,048 tuples/sec | -0.3% | -13.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.553 MB/s | 0.554 MB/s | 0.639 MB/s | -0.2% | -13.5% |
| bs=1000 sw=10 sl=64 | p50 | 1,110,934 us | 1,102,274 us | 976,350 us | +0.8% | +13.8% |
| bs=1000 sw=10 sl=64 | p95 | 1,139,151 us | 1,180,417 us | 1,022,084 us | -3.5% | +11.5% |
| bs=1000 sw=10 sl=64 | p99 | 1,139,151 us | 1,180,417 us | 1,053,520 us | -3.5% | +8.1% |
</details>
<details><summary>Raw CSV</summary>
```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,534.75,200,128000,374,0.228,25607.66,35063.12,35063.12
1,100,10,64,20,2577.44,2000,1280000,776,0.474,125226.60,164273.49,164273.49
2,1000,10,64,20,22087.54,20000,12800000,905,0.553,1110933.97,1139151.46,1139151.46
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org