Skip to content

feat: advisory warnings for realtime provisioning estimates (#8339) - #19026

Open
Vamsi-klu wants to merge 1 commit into
apache:masterfrom
Vamsi-klu:fix/8339-realtime-provisioning-warnings
Open

feat: advisory warnings for realtime provisioning estimates (#8339)#19026
Vamsi-klu wants to merge 1 commit into
apache:masterfrom
Vamsi-klu:fix/8339-realtime-provisioning-warnings

Conversation

@Vamsi-klu

Copy link
Copy Markdown
Contributor

Summary

Adds advisory sanity-check warnings to realtime provisioning estimates (CLI + recommender JSON) and stops hiding over-host-memory matrix cells behind opaque NA.

Issue: Fixes #8339


What is the problem?

The Realtime Provisioning Helper / recommender rule can recommend multi‑GB segments or host/hour combinations whose active memory exceeds -maxUsableHostMemory without calling that out. Previously, over-memory cells were shown as NA, the same token used when numHoursToConsume > retentionHours, which:

  1. Hid the actual computed estimate operators needed to judge risk.
  2. Collapsed two different meanings into one opaque cell.
  3. Gave no structured warning for large segments, high segments-per-host, or over-memory combos.

Issue discussion converged on warnings over hard limits (large segments can be valid for huge volumes after conscious tradeoff).


Why did I do this?

Provisioning mistakes become production OOM, long consumption pauses, and upload timeouts. Advisory warnings preserve recommendation math while making danger visible. Thresholds are named constants for easy review (500 MiB segment size; 5_000 segments/host).


What is the implementation edit?

File Role
NEW RealtimeProvisioningWarnings.java Warning generator + CLI formatter; canonical docs URL; optional hour/host axes for worst-cell labels
NEW RealtimeProvisioningWarningsTest.java Unit tests for thresholds, NA explanation, cell labels, malformed cells
MemoryEstimator.java Always populate matrix cells when hours ≤ retention (including over host memory); expose getMaxUsableHostMemory()
RealtimeProvisioningHelperCommand.java Print warnings after grids; reuse docs URL constant in examples
RealtimeProvisioningRule.java Add "Warnings" entry to recommender output as numbered nested map ("1" → msg, …) fitting existing Map<String,Map<String,String>> payload
TestConfigEngine.java High-ingestion case asserts non-NA cells + presence of Warnings

Warning categories:

  1. Large estimated segment size (> 500 MiB) — includes worst cell coordinates when axes provided
  2. Active memory above max usable host memory
  3. Very high segments queried per host (> 5_000)
  4. Residual NA explanation (hours > retention only)

API note: #8339 asked for an array of warnings; implementation uses a numbered map under the existing matrix bag for backward-compatible JSON shape. Consumers that iterate recommendation keys as matrices should skip "Warnings".

Behavior change: Over-memory cells are no longer NA. Scripts that treated non-NA as “fits in host memory” must read Warnings (or compare to max usable memory) going forward.


What is the impact?

  • Ops: See real estimates + explicit warnings instead of silent danger/NA.
  • Automation: Possible BC for external scripts that assumed non-NA ⇒ safe; documented in this PR.
  • Performance: Negligible (post-estimate scan of small matrices).
  • Docs URL: Uses current site path operate-pinot/tuning/realtime.

What is the testing edit?

./mvnw -pl pinot-controller -Dtest=RealtimeProvisioningWarningsTest,TestConfigEngine test
# RealtimeProvisioningWarningsTest + TestConfigEngine (incl. high ingestion): PASS

Coverage includes: modest grids (no warn), large segment + cell label, memory over host + cell label, high segment count, NA explanation, multiple warnings display format, malformed cell skip, recommender high-ingestion Warnings map.

Not run: full monorepo precommit, multi-node E2E (tooling-only path).


Checklist

  • Draft
  • Backward-compat note for NA semantics
  • Unit tests added/updated

)

Part of local implementation work against apache/pinot issues.
See PR description for problem, rationale, implementation, impact, and testing.
@Vamsi-klu
Vamsi-klu marked this pull request as ready for review July 21, 2026 04:31
@Vamsi-klu

Copy link
Copy Markdown
Contributor Author

Ready for review.

Adds advisory sanity-check warnings for realtime provisioning estimates and stops hiding over-host-memory cells as NA (#8339).

Please take a look when you have a chance:

Notes for review:

  • Recommendation math is unchanged; warnings are advisory only.
  • Over-memory matrix cells are now populated (NA only means hours greater than retention).
  • Warnings key is a numbered nested map under the existing recommender payload shape.

Happy to adjust thresholds, copy, or JSON shape based on feedback.

@codecov-commenter

codecov-commenter commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.80315% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.37%. Comparing base (b9e538a) to head (a63a36a).
⚠️ Report is 107 commits behind head on master.

Files with missing lines Patch % Lines
...ime/provisioning/RealtimeProvisioningWarnings.java 71.81% 11 Missing and 20 partials ⚠️
...mmender/realtime/provisioning/MemoryEstimator.java 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19026      +/-   ##
============================================
+ Coverage     57.10%   65.37%   +8.26%     
- Complexity        7     1405    +1398     
============================================
  Files          2629     3424     +795     
  Lines        156315   216086   +59771     
  Branches      25533    34220    +8687     
============================================
+ Hits          89271   141256   +51985     
- Misses        59423    63440    +4017     
- Partials       7621    11390    +3769     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 65.37% <74.80%> (+8.26%) ⬆️
temurin 65.37% <74.80%> (+8.26%) ⬆️
unittests 65.36% <74.80%> (+8.25%) ⬆️
unittests1 57.09% <ø> (-0.02%) ⬇️
unittests2 37.59% <74.80%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang added ingestion Related to data ingestion pipeline real-time Related to realtime table ingestion and serving labels Aug 5, 2026
@Jackie-Jiang

Copy link
Copy Markdown
Contributor

@Vamsi-klu Can you take a look at the test failures, and rebase to the latest master?

cc @swaminathanmanish @krishan1390 @KKcorps @noob-se7en to take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingestion Related to data ingestion pipeline real-time Related to realtime table ingestion and serving

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add sanity check to the output of Realtime Provisioning Tool

3 participants