Add support for YAML hot deployment and configuration reloading#2959
Open
christiangoerdes wants to merge 14 commits into
Open
Add support for YAML hot deployment and configuration reloading#2959christiangoerdes wants to merge 14 commits into
christiangoerdes wants to merge 14 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds YAML hot-reload: YamlRouterBootstrap to load YAML into a router, router APIs/state to store YAML location and orchestrate reloads, DefaultHotDeployer enable/disable lifecycle updates to manage YAML monitor threads, and YamlHotDeploymentThread that watches tracked files and triggers reloads. ChangesYAML Hot Reload
Sequence Diagram(s)sequenceDiagram
participant RouterCLI
participant YamlRouterBootstrap
participant DefaultRouter
participant DefaultHotDeployer
participant YamlHotDeploymentThread
participant FileSystem
RouterCLI->>YamlRouterBootstrap: loadIntoRouter(router, location)
YamlRouterBootstrap->>DefaultRouter: setYamlConfiguration(location, trackedFiles)
RouterCLI->>DefaultRouter: start()
DefaultRouter->>DefaultHotDeployer: setEnabled(true)
DefaultHotDeployer->>YamlHotDeploymentThread: new YamlHotDeploymentThread(router, hotDeployer)
DefaultHotDeployer->>YamlHotDeploymentThread: start()
YamlHotDeploymentThread->>FileSystem: record initial lastModified timestamps
loop Every polling interval
YamlHotDeploymentThread->>FileSystem: check file timestamps
alt File changed
YamlHotDeploymentThread->>DefaultRouter: reloadYamlConfiguration()
DefaultRouter->>DefaultRouter: shutdown subsystems
DefaultRouter->>YamlRouterBootstrap: loadIntoRouter(router, location)
DefaultRouter->>DefaultRouter: restart components
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/router/YamlRouterBootstrap.java (1)
37-57: 💤 Low valueConsider passing the root source file to
collectTrackedFilesto avoid redundant computation.
getRootSourceFile(location)is called on line 47 when parsing definitions and again on line 62 insidecollectTrackedFiles. The result could be computed once and passed as a parameter.♻️ Suggested refactor
public static void loadIntoRouter(DefaultRouter router, String location) throws Exception { var grammar = new GrammarAutoGenerated(); var registry = new BeanRegistryImplementation(grammar); router.setRegistry(registry); registry.register("router", router); + Path rootSourceFile = getRootSourceFile(location); List<BeanDefinition> definitions = registry.parseYamlBeanDefinitions( router.getResolverMap().resolve(location), grammar, - getRootSourceFile(location) + rootSourceFile ); BeanDefinitions beanDefinitions = new BeanDefinitions(definitions); beanDefinitions.ensureSingleGlobalDefinition(); beanDefinitions.getConfigDefinition().ifPresent(configBd -> router.applyConfiguration((Configuration) registry.resolve(configBd.getName()))); - router.setYamlConfiguration(location, collectTrackedFiles(location, definitions)); + router.setYamlConfiguration(location, collectTrackedFiles(rootSourceFile, definitions)); registry.finishStaticConfiguration(); router.getConfiguration().setBaseLocation(location); } -private static List<Path> collectTrackedFiles(String location, List<BeanDefinition> definitions) { +private static List<Path> collectTrackedFiles(Path rootSourceFile, List<BeanDefinition> definitions) { LinkedHashSet<Path> trackedFiles = new LinkedHashSet<>(); - Path rootSourceFile = getRootSourceFile(location); if (rootSourceFile != null) { trackedFiles.add(rootSourceFile.toAbsolutePath().normalize()); }Also applies to: 59-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/router/YamlRouterBootstrap.java` around lines 37 - 57, Compute getRootSourceFile(location) once in loadIntoRouter and pass that File (or Path) into collectTrackedFiles instead of calling getRootSourceFile(location) twice; update the call site in loadIntoRouter to store the result in a local (e.g., rootSourceFile) and use that variable for both registry.parseYamlBeanDefinitions(..., getRootSourceFile(location)) and router.setYamlConfiguration(location, collectTrackedFiles(location, definitions)); adjust collectTrackedFiles signature if needed to accept the precomputed root source file and update any usages of collectTrackedFiles accordingly.core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (2)
92-94: 💤 Low valueDouble-stop of
reinitializerwhen router stops.
DefaultRouter.stop()callshotDeployer.stop()(which now callsrouter.getReinitializer().stop()at line 93), and thenDefaultRouter.stop()callsreinitializer.stop()again at line 312. This results in callingstop()twice on the same reinitializer.If
RuleReinitializer.stop()is idempotent this is harmless but inefficient. If not, it could cause issues. Consider removing this call fromDefaultHotDeployer.stop()since the router already handles it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java` around lines 92 - 94, DefaultHotDeployer.stop() currently invokes router.getReinitializer().stop(), which causes the same reinitializer to be stopped again when DefaultRouter.stop() later calls reinitializer.stop(); remove the explicit router.getReinitializer().stop() call from DefaultHotDeployer.stop() so the router exclusively manages RuleReinitializer lifecycle (keep the null-check/guarding logic around router but do not call getReinitializer().stop()), and add a brief comment in DefaultHotDeployer.stop() noting that DefaultRouter.stop() handles reinitializer shutdown to prevent double-stop.
108-114: ⚡ Quick win
setEnabled(false)does not stop the running hot-deployment thread.When
enabledis set tofalse, the code only updates the field. The hot-deployment thread will continue running until it next checkshotDeployer.isEnabled()in its polling loop (up to 1 second later). For immediate shutdown semantics, callstop()when disabling.Suggested fix
`@Override` public void setEnabled(boolean enabled) { this.enabled = enabled; if (enabled && router != null) { startInternal(); + } else if (!enabled) { + stop(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java` around lines 108 - 114, The setter setEnabled(boolean enabled) only flips the flag and starts the deployer when enabling, but does not stop the running hot-deployment thread immediately when disabling; modify setEnabled to call stop() when enabled is false (and router != null if you need same null-check as start) so the hot-deployer thread is shut down immediately; update setEnabled to invoke stop() before/after updating this.enabled (preserve thread-safety semantics used elsewhere) and reference DefaultHotDeployer.startInternal(), DefaultHotDeployer.stop(), the enabled field and the hotDeployer.isEnabled() check in the polling loop when making the change.core/src/main/java/com/predic8/membrane/core/router/hotdeploy/YamlHotDeploymentThread.java (1)
39-44: ⚡ Quick winConsider making this a daemon thread.
Non-daemon threads prevent JVM shutdown. If the application wants to terminate but this thread is still running (e.g., waiting in
sleep()), the JVM will hang. Setting daemon mode allows clean shutdown.Suggested fix
public YamlHotDeploymentThread(DefaultRouter router, DefaultHotDeployer hotDeployer) { super("yaml-hotdeploy"); + setDaemon(true); this.router = router; this.hotDeployer = hotDeployer; refreshTrackedFiles(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/YamlHotDeploymentThread.java` around lines 39 - 44, The YamlHotDeploymentThread constructor currently creates a non-daemon thread which can prevent JVM shutdown; update the constructor in class YamlHotDeploymentThread so the thread is marked as a daemon (call setDaemon(true))—do this in the YamlHotDeploymentThread(DefaultRouter, DefaultHotDeployer) constructor (before the thread might be started) so that this background thread won’t block JVM exit while still calling refreshTrackedFiles() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java`:
- Around line 117-119: The yamlConfigurationLocation and yamlTrackedFiles fields
are currently synchronized on this in their getters/setters but are read under
synchronized(lock) in reloadYamlConfiguration(), causing inconsistent locking;
update the getters/setters for yamlConfigurationLocation and yamlTrackedFiles to
use synchronized(lock) instead of synchronized(this), and add `@GuardedBy`("lock")
annotations to those fields (similar to reloading) so all accesses (fields and
methods including reloadYamlConfiguration(), getYamlConfigurationLocation(),
setYamlConfigurationLocation(), getYamlTrackedFiles(), setYamlTrackedFiles())
consistently synchronize on the same lock.
- Around line 496-510: The reload leaves the router stopped if loadIntoRouter()
or start() fails because shutdownForYamlReload() is called before attempting the
load; change the flow so you either perform the YAML load/validation before
shutting down or, at minimum, attempt to recover on failure: before calling
shutdownForYamlReload() keep whatever state/reference is needed, then wrap
YamlRouterBootstrap.loadIntoRouter(this, location) and start() in a try, and in
the catch block attempt to restart the previous runtime by invoking start() (or
restore the backed-up state) and log both the original error and the recovery
attempt/result; reference shutdownForYamlReload(), resetForYamlReload(),
YamlRouterBootstrap.loadIntoRouter(), and start() to locate the code to modify.
- Around line 535-540: resetForYamlReload() violates the `@GuardedBy`("lock")
contract by setting the guarded field initialized = false without holding lock;
either acquire the same lock in resetForYamlReload() (wrap the body in
synchronized(lock)) or move the initialized = false assignment into the
synchronized(lock) block inside reloadYamlConfiguration() so that modifications
to initialized (and any other guarded fields like mainComponents, configuration,
reinitializer) always occur while holding lock; update the method
resetForYamlReload() or the synchronized section in reloadYamlConfiguration()
accordingly to ensure the lock protects initialized.
In
`@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java`:
- Line 34: The field 'enabled' has a race/visibility issue; change its
declaration (private boolean enabled = true) to be volatile (private volatile
boolean enabled = true) so updates made in setEnabled(...) are immediately
visible to threads reading it in startInternal() and other methods (e.g.,
isEnabled()); you can then keep setEnabled() and isEnabled() unsynchronized, or
alternatively protect all reads/writes with synchronized(lock) if you prefer
explicit locking—pick one approach and apply it consistently across all accesses
to 'enabled'.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java`:
- Around line 92-94: DefaultHotDeployer.stop() currently invokes
router.getReinitializer().stop(), which causes the same reinitializer to be
stopped again when DefaultRouter.stop() later calls reinitializer.stop(); remove
the explicit router.getReinitializer().stop() call from
DefaultHotDeployer.stop() so the router exclusively manages RuleReinitializer
lifecycle (keep the null-check/guarding logic around router but do not call
getReinitializer().stop()), and add a brief comment in DefaultHotDeployer.stop()
noting that DefaultRouter.stop() handles reinitializer shutdown to prevent
double-stop.
- Around line 108-114: The setter setEnabled(boolean enabled) only flips the
flag and starts the deployer when enabling, but does not stop the running
hot-deployment thread immediately when disabling; modify setEnabled to call
stop() when enabled is false (and router != null if you need same null-check as
start) so the hot-deployer thread is shut down immediately; update setEnabled to
invoke stop() before/after updating this.enabled (preserve thread-safety
semantics used elsewhere) and reference DefaultHotDeployer.startInternal(),
DefaultHotDeployer.stop(), the enabled field and the hotDeployer.isEnabled()
check in the polling loop when making the change.
In
`@core/src/main/java/com/predic8/membrane/core/router/hotdeploy/YamlHotDeploymentThread.java`:
- Around line 39-44: The YamlHotDeploymentThread constructor currently creates a
non-daemon thread which can prevent JVM shutdown; update the constructor in
class YamlHotDeploymentThread so the thread is marked as a daemon (call
setDaemon(true))—do this in the YamlHotDeploymentThread(DefaultRouter,
DefaultHotDeployer) constructor (before the thread might be started) so that
this background thread won’t block JVM exit while still calling
refreshTrackedFiles() as before.
In
`@core/src/main/java/com/predic8/membrane/core/router/YamlRouterBootstrap.java`:
- Around line 37-57: Compute getRootSourceFile(location) once in loadIntoRouter
and pass that File (or Path) into collectTrackedFiles instead of calling
getRootSourceFile(location) twice; update the call site in loadIntoRouter to
store the result in a local (e.g., rootSourceFile) and use that variable for
both registry.parseYamlBeanDefinitions(..., getRootSourceFile(location)) and
router.setYamlConfiguration(location, collectTrackedFiles(location,
definitions)); adjust collectTrackedFiles signature if needed to accept the
precomputed root source file and update any usages of collectTrackedFiles
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1a19c3c-8c30-47c3-8bde-8533f0bb2cf4
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.javacore/src/main/java/com/predic8/membrane/core/router/YamlRouterBootstrap.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/YamlHotDeploymentThread.java
|
This pull request needs "/ok-to-test" from an authorized committer. |
Collaborator
Author
|
/ok-to-test |
predic8
requested changes
May 27, 2026
…oader interface and enhancing hot deployment logic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor