Jitter StatusPlugin report interval between min/max delay - #159
Conversation
08f6630 to
3b51a0a
Compare
| @JsonProperty("min_delay_seconds") | ||
| private Integer minDelaySeconds = 30; | ||
|
|
||
| @JsonProperty("max_delay_seconds") |
There was a problem hiding this comment.
Defaulting maxDelaySeconds to a non-null 30 makes the "unset max → 2 * min" fallback in StatusPlugin.start() unreachable through config, and turns a min-only configuration into a validation error.
Jackson only invokes a setter for keys physically present in the config, so an omitted max_delay_seconds retains this 30 default rather than becoming null. As a result:
maxDelaySeconds = minDelaySeconds * 2instart()can only fire if a caller explicitly passesnull(as the test does) — never via normal YAML/JSON — contradicting the PR description's "ifmax_delay_secondsis unset, it now defaults to2 * min_delay_seconds."- A user who sets only
min_delay_seconds: 60(max omitted) keepsmax = 30, sovalidate()fails withmin_delay_seconds (60) cannot be greater than max_delay_seconds (30)at startup — even though they never configured a max.
DecisionLogsConfig sidesteps this with split defaults (300/600), and ReportingConfig uses null/null. Consider defaulting maxDelaySeconds (and probably minDelaySeconds) to null here and applying the 30 default in start(), so the documented min-only fallback actually works.
|
Thanks @sspaink — good catch. |
| int minDelaySeconds = | ||
| (status.getMinDelaySeconds() != null) ? status.getMinDelaySeconds() : 30; | ||
| int maxDelaySeconds = | ||
| (status.getMaxDelaySeconds() != null) ? status.getMaxDelaySeconds() : minDelaySeconds * 2; |
There was a problem hiding this comment.
When only max_delay_seconds is configured and it's below 30, min defaults to 30, so min > max and the report just fires every 30s — the configured max is silently ignored. validate() doesn't catch it either, since it only compares the two when both are non-null. Consider validating this case or defaulting min to max when only max is set.
| } | ||
|
|
||
| // Validate delay settings | ||
| if (statusConfig.getMinDelaySeconds() != null && statusConfig.getMaxDelaySeconds() != null) { |
There was a problem hiding this comment.
This only checks min > max, but doesn't reject negative values like BundleDownloader.validatePolling does (it guards min < 0 / max < 0). A negative min_delay_seconds passes validation and produces a negative delay, which the scheduler treats as 0 — so the chain busy-loops reporting status. Suggest adding the same >= 0 guards.
StatusPlugin.start() scheduled status reports via scheduleAtFixedRate at a hardcoded 30s interval, with no way to configure it and no jitter, so every instance in a fleet reports on the same cadence. The twin decision-log issue (open-policy-agent#78) and this SDK's BundleDownloader already establish the chained random-delay pattern for spreading load across instances. Add min_delay_seconds/max_delay_seconds to Config.StatusConfig (defaulting to 30/30, preserving the previous fixed interval when unconfigured) and wire them through StatusPlugin -> Status, mirroring DecisionLogsConfig. Replace scheduleAtFixedRate with scheduleNextReport, mirroring BundleDownloader.scheduleNextPoll and DecisionLogPlugin.scheduleNextFlush: each report re-schedules itself with a uniformly random delay in [min, max], swallowing exceptions from reportStatus() (which already logs internally) so the chain keeps running, and stopping cleanly on RejectedExecutionException after shutdown. The immediate report on startup is preserved via a separate zero-delay schedule() call before the chain starts, matching BundleDownloader.startPolling()'s immediate-download-then-poll structure - so there's no regression in time-to-first-report. When max_delay_seconds is unset, it now defaults to 2x min_delay_seconds, consistent with open-policy-agent#78's DecisionLogPlugin fallback. Also switch the plugin's scheduler to BundleDownloader.newPollScheduler(...), matching BundlePlugin, DiscoveryPlugin, and the DecisionLogPlugin fix in open-policy-agent#78. Note: I checked OPA Go's current status plugin source and it has no standalone min_delay_seconds/max_delay_seconds of its own - status reports there are triggered by the bundle/discovery plugin's own polling rather than an independent timer - so there is no canonical upstream number to mirror for the default interval. I kept this SDK's existing 30s default unchanged for unconfigured setups (no behavior change), while adding full jitter support once a wider [min, max] range is configured, in the spirit of this issue and its open-policy-agent#78 twin. Fixes open-policy-agent#80 Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
Jackson only invokes setters for keys present in config, so a non-null field default made the start()-time unset-max → 2*min fallback unreachable via YAML. Mirror ReportingConfig: leave min/max null on the config bean and apply 30 / 2*min in StatusPlugin.start(). Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
Mirror BundleDownloader.validatePolling's >= 0 guards so a negative min_delay_seconds cannot busy-loop the status scheduler. When only max_delay_seconds is set below the 30s default min, default min to min(30, max) so the configured max is not silently ignored. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks @sspaink — addressed both points (and rebased onto
Tests: |
16e9b48 to
f3fe89d
Compare
Problem
StatusPlugin.start()scheduled status reports viascheduleAtFixedRate(...)at a hardcoded 30s interval, with no way to configure it and no jitter. Every instance in a fleet therefore reports on the same cadence. This is the twin of #78 (DecisionLogPlugin), and this SDK'sBundleDownloaderalready implements the chained random-delay pattern for bundle polling.Approach
Mirrors
BundleDownloader.scheduleNextPolland this repo's own #78 fix forDecisionLogPlugin:min_delay_seconds/max_delay_secondstoConfig.StatusConfig(previously it only hadconsoleandservice), defaulting to30/30— see note below on why.minDelaySeconds/maxDelaySecondsfields + getters/setters to the innerStatusPlugin.Statusclass, and wired them through ininitialize().min_delay_seconds <= max_delay_secondscheck tovalidate(), matching the existing pattern inDecisionLogPlugin.validate().scheduler.scheduleAtFixedRate(...)with a new privatescheduleNextReport(minDelay, maxDelay)that re-schedules itself after each report with a uniformly random delay in[minDelay, maxDelay](ThreadLocalRandom), swallowingExceptionfromreportStatus()(which already logs internally) so the chain keeps running, and stopping cleanly onRejectedExecutionExceptionafter shutdown — same structure asBundleDownloader.scheduleNextPolland theDecisionLogPlugin.scheduleNextFlushadded in Jittered upload interval #78.scheduler.schedule(...)call before starting the chain, mirroringBundleDownloader.startPolling()'s "download immediately, then start the chained poll" structure — so there's no regression in time-to-first-report.max_delay_secondsis unset, it now defaults to2 * min_delay_seconds, consistent with the fallback added forDecisionLogPluginin Jittered upload interval #78.BundleDownloader.newPollScheduler("opa-status-scheduler"), matchingBundlePlugin,DiscoveryPlugin, and the Jittered upload interval #78DecisionLogPluginchange.Note on defaults: I checked OPA Go's current
v1/plugins/status/plugin.goand itsConfigstruct has nomin_delay_seconds/max_delay_secondsfields at all — status reports there are triggered by the bundle/discovery plugin's own polling (Triggermode) rather than an independent timer, so there's no canonical upstream number to mirror for a standalone status interval default (this differs from decision logs, where OPA Go does havedefaultMinDelaySeconds = 300/defaultMaxDelaySeconds = 600inv1/plugins/logs/plugin.go, matching what's already in this SDK'sDecisionLogsConfig). Given that, I kept this SDK's existing 30s interval as the default when unconfigured (min = max = 30, deterministic, zero behavior change for existing users), while adding full jitter support once a wider[min, max]range is configured — which is the actual ask of this issue.Tests
Added to
StatusPluginTest:validate_delaySecondsInvalid_returnsError/validate_delaySecondsEqual_returnsNoErrors— validation edge cases formin <= max.configDefaults_delaySecondsAreCorrect— confirms the30/30default.configBuilder_setsAllFields— updated to also cover the new delay setters.start_periodicReport_usesJitteredChainedSchedule— setsmin == max == 1sfor a deterministic interval and verifies at least 3"Status: %s"console logs (immediate + ~1s + ~2s), proving the schedule re-chains itself.start_onlyMinDelayConfigured_defaultsMaxToTwiceMin— sets onlymin_delay_seconds = 1withmax_delay_secondsexplicitlynull, and verifies at least 2 reports within 2.5s, proving the fallback is2 * minrather than an unrelated default that wouldn't fire in the window.Also added
config_statusDefaultscoverage inConfigTestfor the new fields.Ran locally with JDK 17 / Gradle:
StatusPluginTest: 20/20 pass (14 pre-existing + 6 new/updated).ConfigTest: 14/14 pass. Full multi-module./gradlew testand:opa-services:checkstyleMainalso pass with zero violations in the changed files.Fixes #80
Fixes #80