Skip to content

Reconfiguration Trigger SPI#117

Open
SamBarker wants to merge 8 commits into
kroxylicious:mainfrom
SamBarker:feat/trigger-spi-proposal
Open

Reconfiguration Trigger SPI#117
SamBarker wants to merge 8 commits into
kroxylicious:mainfrom
SamBarker:feat/trigger-spi-proposal

Conversation

@SamBarker

@SamBarker SamBarker commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary

  • Defines a pluggable SPI for triggering KafkaProxy.reconfigure(), discovered via ServiceLoader
  • Formalises the trigger responsibilities established during the Proposal 083 review (failure policy, rollback, concurrency handling, debouncing, config persistence)
  • Three interfaces: ReconfigurationTrigger, ReconfigurationTriggerFactory, ReconfigurationTriggerContext
  • Configuration follows the existing type + config plugin pattern
  • Includes a file watcher sketch to demonstrate the SPI is sufficient for the common case

Context

Proposal 083 delivered KafkaProxy.reconfigure(Configuration) but explicitly deferred trigger mechanisms. The PR #83 discussion explored file watchers, HTTP endpoints, and operator callbacks, ultimately agreeing to decouple the trigger from the core reconfiguration machinery. This proposal provides the extension point that makes hot reload usable in the shipped binary and extensible for custom control planes.

🤖 Generated with Claude Code

Defines a pluggable SPI for triggering KafkaProxy.reconfigure(),
building on the trigger responsibilities established during the
Proposal 083 review. Covers the SPI interfaces, configuration model,
lifecycle, and the full trigger responsibility contract.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
@SamBarker
SamBarker requested a review from a team as a code owner June 24, 2026 23:07
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Replace file-specific parseConfiguration(Path) with source-agnostic
parseConfiguration(InputStream). Add validateConfiguration() for
pre-flight validation before applying. Keep configFilePath() with
clearer rationale as the path the proxy was booted from.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Triggers implementing failure policies (shut down on any failure,
last-resort after failed rollback) need the ability to initiate
proxy shutdown. Without this, the canonical patterns from Proposal
083 cannot be expressed through the SPI.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Replace Configuration with Snapshot as the type triggers provide to
reconfigure(). This decouples triggers from the configuration format:
triggers produce a source-agnostic Snapshot, the runtime handles
parsing internally. parseConfiguration() is removed — parsing is no
longer a trigger concern.

The Snapshot abstraction is promoted from Proposal 096's internal
runtime type to public API, ensuring the trigger SPI won't need to
change when multi-file configuration lands.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
└── Proxy completes shutdown
```

**Startup ordering.** The trigger is started *after* the proxy has completed startup and all virtual clusters are serving. This is why the `ReconfigurationTrigger` interface separates construction (`create()`) from activation (`start()`): the factory creates the trigger during proxy initialisation, but the trigger must not begin watching for changes — or call `reconfigure()` — until the proxy is ready. Without this separation, a file watcher trigger could detect the existing configuration file immediately on construction and attempt a `reconfigure()` before the proxy has loaded its initial configuration, which would throw `IllegalStateException` per Proposal 083.

@robobario robobario Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I was trying to understand was why we need a different mechanism for reload vs initial Configuration of the Proxy. I squint at it and think "oh, shouldn't initialization just be a kind of reload going from something to nothing", pointed Claude at it and think its explanation is sane:

The proposal does explain part of this (the create()/start() split), but your question goes deeper — why does initial load go through a separate startup path at all, rather than through reconfigure()?

The key reason is that reconfigure() semantics are fundamentally "converge from a prior running state." The proxy needs to already have virtual clusters bound to ports, initialized subsystems, etc. to know what to change. Without a running state, reconfigure() throws
IllegalStateException per 083. So you can't bootstrap from nothing through reconfigure().

For the file watcher specifically, the code similarity is real — both initial load and change detection read the same file and call parseConfiguration(InputStream). But two things make unification difficult:

  1. The trigger doesn't know what to watch until after startup — it gets configFilePath() from the context, which is determined during proxy startup. There's a bootstrapping dependency.
  2. Failure semantics differ — startup failure means "don't start the proxy"; reconfigure failure has a policy (rollback, best-effort, shutdown). Conflating them would force an awkward policy at startup time.

There's also a more subtle point: static configuration (the reconfigurationTrigger section itself, listen ports, etc.) is explicitly out of scope for reconfigure(). This means the startup path and the reconfigure path will always handle different sets of config, so they can't be fully unified regardless.

The duplication in the file watcher case (open file → parse → do something with Config) is minor enough that it doesn't seem worth collapsing the abstraction boundary. The two "do something" operations are genuinely different enough in semantics and failure handling that keeping them separate is the right call. Whether that's worth a comment in the proposal is a fair question though — the lifecycle section explains the create()/start() split but doesn't explicitly address why initial load isn't routed through reconfigure().

So yeah, there's a granularity difference where the proxy config has some global config, where reconfigure is really meant to be scoped to the virtual clusters. That and failure semantics are convincing.

Does that match your ideas @SamBarker? It's not really something new in this proposal, it's just been on my mind during the reload implementation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had something similar on my mind, but haven't explored too deeply. I'm not entirely buying Claude's thinking. I'm about to push up a big(ish) change around how this interacts with config v2. I think there might be a way to pick apart the fixed config from the virtual cluster config and merge the startup and reload paths.

@robobario robobario Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though maybe there is an implication here, like I'm wondering, should the user be able to plug in something like a Config source? A single place where we can obtain the startup Config, and it can be hooked into the hot reload trigger lifecycle? So there's a single component that knows about the File (or other sources like HTTP etc), how to parse it and integrate with hot reload and startup.

I can see that there's a bootstrapping problem, if we define the config sources or reload triggers in the main config file we have to load this to get at the SPIs 🤔 Maybe it implies we need to have a bootstrap config where these fundamental configuration options can be defined, separate from our existing proxy config files? By default, if the user didn't supply one, it would be implied that they are using a Configuration File source identified by the other command line args

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well that escalated quickly...

Ok see what you and your Claude make of that update... ;)

The trigger is now the sole source of reloadable configuration,
including the initial load at startup. The proxy starts in an empty
state (bootstrap infrastructure only) and the trigger's first
reconfigure() call brings virtual clusters to life. This eliminates
the two-path inconsistency where the constructor loaded initial
config and triggers handled subsequent changes.

Key changes:
- Logical split between bootstrap (static) and reloadable (via Snapshot)
- start() performs initial load synchronously, then sets up watching
- Failure to start exits the proxy (no VCs = not useful)
- OutOfScopeChangeException no longer relevant for trigger-driven reconfig
- New rejected alternative: runtime-loads-initial model

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Rename ReconfigurationTrigger → VirtualClusterConfigController
throughout, borrowing from the Kubernetes controller pattern: a
control loop that watches desired state, detects drift, and
reconciles. Rename bootstrap/reloadable config split to process
configuration vs virtual cluster configuration.

Add rejected alternative for separate ConfigurationSource and
controller plugins — the coupling between source and controller
is deployment-specific and no single SPI boundary works for all
cases (file watcher shares a path, HTTP PUT body *is* the source,
partial config needs merging with current state).

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
validate() now returns a ValidationResult with errors deduplicated by
root cause instead of throwing ConfigurationException. This gives
controllers a uniform error-handling pattern (check isValid()) rather
than mixing try/catch for validation with whenComplete() for
reconfiguration, and prevents cascading errors (e.g. one bad plugin
type referenced by N virtual clusters producing N identical errors).

configFilePath() is removed from VirtualClusterConfigControllerContext.
It was a file-specific method on a source-agnostic interface, left over
from before the process/virtual-cluster configuration split. File-based
controllers get their path from their own factory config.

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
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.

2 participants