Skip to content

Add accounting scopes for external query execution - #19542

Draft
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/native-execution-accounting
Draft

xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/native-execution-accounting

Conversation

@xiangfu0

Copy link
Copy Markdown
Contributor

External query execution cannot call Pinot's cooperative Java accounting checkpoints while its query thread is outside Java. Calling the existing checkpoint from a monitor instead samples the monitor thread, and blocking the shared monitor to honor a pause prevents it from observing resume requests.

This adds an optional, owner-bound ExternalExecutionSampler to ThreadAccountant. Supported accountants publish the original platform query thread's CPU and JVM allocation deltas into the existing tracker and expose a nonblocking pause observation. Closing the scope on the owner drains in-flight sampling before the query context is cleared.

Compatibility and scope

  • The SPI addition is a default method returning null. Unsupported accountants and custom subclasses must explicitly implement their policy before external execution can use them; callers must retain Java checkpoints or reject that execution path when capture returns null.
  • Existing Java sampling paths and query accounting totals are retained. There are no query wire, configuration, segment, or distributed-state changes.
  • Synchronous external work on the owner platform thread is included in its CPU measurement. CPU on external helper threads and native heap allocations are not measured by this hook and require separate accounting. Virtual owners are rejected by the CPU/allocation tracker.
  • This PR supplies the accounting extension point, not an external engine or its cancellation/deadline implementation. It adds no benchmark source and makes no performance claim.

Integration example

Capture after opening the query's accounting context, on the thread that will execute external work. The example below is schematic: sharedMonitor, registration, and control are supplied by the consuming engine; closing the registration must unregister and drain its callback.

ExternalExecutionSampler sampler = accountant.captureExternalExecutionSampler();
if (sampler == null) {
  throw new UnsupportedOperationException("Accountant requires Java checkpoints");
}
try (sampler) {
  var registration = sharedMonitor.register(() -> {
    try {
      sampler.sampleUsage();
      control.setPaused(sampler.isPaused());
    } catch (RuntimeException failure) {
      control.cancelWithFailure(failure);
    }
  });
  try {
    executeSynchronouslyOutsideJava(control);
  } finally {
    registration.close(); // Unregister and drain before freeing control state.
    sampler.sampleUsage(); // Owner's final sample; try-with-resources still closes on failure.
  }
}
// Only now clear the owner query context and release the external control state.

The monitor must keep observing while execution is paused so it can forward resume requests. Sampling failures must stop execution and propagate to the query. The consuming engine remains responsible for deadlines, cancellation, and its own allocation budget. Do not independently sample/reset/clear the owning accountant while this scope is active. The same lifecycle is documented on ExternalExecutionSampler.

Validation

JDK 25:

./mvnw -pl pinot-core -am \
  -Dtest=ExternalExecutionSamplerTest,ExternalThreadAccountingTest \
  -Dsurefire.failIfNoSpecifiedTests=false install

All 16 selected reactor modules succeeded. All 9 new tests passed (3 SPI, 6 core), covering original-thread CPU/allocation attribution, final accounting cleanup, real pause/resume state, draining and owner-only close, and unsupported/custom/virtual/foreign-context rejection. These tests exercise the Java management counters and lifecycle; they do not benchmark or validate an external engine.

Affected-module Spotless, Checkstyle, license checks, and warning-enabled test-compile passed. The compiler emitted no warnings for the changed classes.

The additional warning-enabled reactor test-compile check stops in unchanged pinot-segment-local code at ZstandardDecompressor.java:51 because org.jetbrains.annotations.NotNull is unavailable. This does not affect the successful normal reactor install/test run above.

@xiangfu0 xiangfu0 added extension-point Adds or modifies an extension/SPI point feature New functionality labels Sep 13, 2026
@codecov-commenter

codecov-commenter commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.78%. Comparing base (5771d6a) to head (8cc96ea).

Files with missing lines Patch % Lines
...ot/spi/accounting/ThreadResourceUsageProvider.java 23.07% 5 Missing and 5 partials ⚠️
...e/pinot/spi/accounting/ThreadResourceSnapshot.java 50.00% 2 Missing and 2 partials ⚠️
...counting/HeapUsagePublishingAccountantFactory.java 0.00% 3 Missing ⚠️
...ore/accounting/ResourceUsageAccountantFactory.java 50.00% 1 Missing and 1 partial ⚠️
...not/core/accounting/ThreadResourceTrackerImpl.java 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19542      +/-   ##
============================================
+ Coverage     67.72%   67.78%   +0.06%     
  Complexity     1450     1450              
============================================
  Files          3490     3491       +1     
  Lines        225032   225095      +63     
  Branches      35527    35538      +11     
============================================
+ Hits         152393   152576     +183     
+ Misses        60612    60463     -149     
- Partials      12027    12056      +29     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 67.78% <66.66%> (+0.06%) ⬆️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 67.78% <66.66%> (+0.06%) ⬆️
unittests 67.78% <66.66%> (+0.06%) ⬆️
unittests1 57.90% <66.66%> (+0.08%) ⬆️
unittests2 39.48% <3.17%> (-0.02%) ⬇️

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.

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

Labels

extension-point Adds or modifies an extension/SPI point feature New functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants