Skip to content

Let SamplingManagerConfig name the executor sampling groups run on - #2104

Merged
kevinherron merged 2 commits into
integration/1.2from
sampling-group-executor
Oct 6, 2026
Merged

kevinherron merged 2 commits into
integration/1.2from
sampling-group-executor

Conversation

@kevinherron

@kevinherron kevinherron commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Sampling groups run every read access refresh and every sample call on the server's executor. When a sampler's reads block, each turn holds one of the server's shared threads until the read returns. The default AddressSpaceSamplingGroup over a slow device does this, and so does a driver with a synchronous protocol. With many interval groups across many address spaces, that blocking competes with service requests. The only way to move the work was OpcUaServerConfigBuilder.setExecutor, and that moves every service request with it.

This PR adds an optional executor to SamplingManagerConfig. When it is set, each group runs its turns there. When it is null (the default), groups keep using the server's executor, so existing servers behave as before.

@Override
protected SamplingManagerConfig samplingManagerConfig() {
  // For example Executors.newVirtualThreadPerTaskExecutor() on Java 21+, created once by the address space.
  return SamplingManagerConfig.defaults().withExecutor(samplingExecutor);
}

Milo still targets Java 17. The config takes a plain java.util.concurrent.Executor, so a server on Java 21 or later can pass a virtual-thread-per-task executor without Milo referencing virtual threads.

What changes and what stays

  • SamplingGroup resolves its executor in configure, which SamplingManager calls before a group starts. Every path into sample uses that executor: the timed cycle, the debounced initial sample, and a turn handed off after the previous one completed.
  • Timers stay on the server's scheduled executor. This covers the next cycle, the initial-sample debounce, the overrun watchdog, and the turn handoff. Those tasks only dispatch to the executor or log.
  • The turn model does not depend on the executor. A group still runs one refresh-and-sample at a time, enforced by its own lock, so a thread-per-task executor is safe and the executor need not be serial or bounded.
  • The executor must run tasks on threads of its own. An executor that runs them on the calling thread would run every turn on the server's scheduled executor. The docs say so; the code cannot detect it.
  • A rejected turn no longer stops a group. Before this PR, a RejectedExecutionException from the dispatch escaped into the scheduled future and was lost. A rejected cycle never scheduled the next one. A rejected handoff never released the turn, because releaseTurn takes the turn before dispatching it, so the group could never sample again. Now a rejection is logged and skipped. A rejected cycle schedules the next one. A rejected handoff releases the turn, and schedules the next cycle if it was a cycle's. A rejected initial sample leaves its items for the next cycle. This applies to the server's executor too, but a bounded or closed caller-owned executor makes rejection much more likely.
  • The caller owns the executor. The manager never shuts it down.
  • SamplingManagerConfig gains a seventh record component. The type is new in 1.2 and has not been released, so no released API changes.

On Java 21 to 23, synchronized still pins a virtual thread to its carrier (JEP 491 removed that in Java 24). The group's own locked sections are short and never block. A sampler that blocks inside its own synchronized code would still pin on those versions.

Docs

The SamplingManagerConfig and SamplingGroup Javadoc, the sampling package-info, docs/features/sampling.md (configuration table, a short example, and the troubleshooting row for blocked worker threads), and the 1.2.0 release notes now describe the setting.

Verification

SamplingGroupTest.everyTurnRunsOnTheConfiguredExecutor configures an executor that marks when it is running a task. It drives an initial sample that holds the turn, a cycle that comes due meanwhile and runs on the handoff, and a cycle on time. It checks that all three sample calls ran inside the configured executor. It also checks that the executor received exactly four tasks: those three turns plus the cycle that found the turn held. That shows timers and the watchdog do not go through it. The existing tests cover the default path, where the executor is null and groups run on the server's executor.

aRejectedCycleIsSkippedAndTheNextOneIsScheduled and aRejectedHandoffReleasesTheTurn drive an executor that rejects. They check that the next cycle is still scheduled and that the group samples again once the executor accepts. Both fail on the first commit's SamplingGroup, where the rejection escapes the scheduler task.

Run locally on e2ff7f3:

  • mvn -q spotless:apply and mvn -q clean compile
  • mvn -q -pl opc-ua-sdk/sdk-server -am verify filtered to the sampling package, ManagedAddressSpaceSamplingTest, and SubscriptionModelTest: 78 tests, 0 failures

Sampling groups ran every refresh and sample on the server's
executor. A sampler whose reads block, such as the default
AddressSpaceSamplingGroup over a slow device, held one of the server's
shared threads for the whole turn. The only way to move that work was
OpcUaServerConfigBuilder.setExecutor, which moves every service request
along with it.

SamplingManagerConfig now has an optional executor. When it is set,
each group runs its turns there; when it is null, the default, groups
keep using the server's executor. Timers stay on the server's
scheduled executor. A group runs one turn at a time whatever the
executor, so a thread-per-task executor works, including a virtual
thread per turn on Java 21 or later. The caller owns the executor and
the manager never shuts it down.
@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Adds executor configuration to sampling group scheduling.

The PR appears safe to merge; no outstanding blocking finding remains.

Summary

The PR lets sampling groups use a caller-owned executor while retaining the server executor as the default.

  • Rejection handling now skips a rejected turn and preserves subsequent cycle scheduling.
  • Documentation specifies that the configured executor must run work off the scheduler thread.
  • Tests cover configured dispatch and recovery from rejected cycles and handoffs.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  T[Server scheduler] --> D[Dispatch turn]
  D -->|accepted| E[Configured executor or server executor]
  E --> S[Refresh and sample]
  S --> N[Schedule next cycle or hand off due turn]
  D -->|rejected cycle| R[Schedule next cycle]
  D -->|rejected handoff| H[Release turn]
Loading

Reviews (2) · Last reviewed commit: "Keep a sampling group alive when its exe..."

A group dispatches each turn from a scheduler task with
executor.execute. A RejectedExecutionException there, from a full
bounded pool or an executor already shut down, escaped into the
scheduled future and was lost. A rejected cycle never scheduled the
next one, so the group stopped sampling. A rejected handoff was
worse: releaseTurn takes the turn before dispatching it, so the turn
was never released and no cycle or initial sample could run again.

A rejection is now logged and skipped. A rejected cycle schedules the
next one. A rejected handoff releases the turn, and schedules the next
cycle if it was a cycle's. A rejected initial sample leaves its items
pending for the next cycle.

The executor docs also say it must run tasks on threads of its own.
An executor that runs them on the calling thread would run every turn
on the server's scheduled executor.
@kevinherron

Copy link
Copy Markdown
Contributor Author

@greptileai review

e2ff7f3 handles a RejectedExecutionException from the sampling executor and documents that the executor must use its own threads. Replies are on each thread.

@kevinherron
kevinherron merged commit d680f25 into integration/1.2 Oct 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant