-
Notifications
You must be signed in to change notification settings - Fork 149
Router sync registry fix #2540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Router sync registry fix #2540
Conversation
…, update roadmap and dependencies
…al for improved immutability
📝 WalkthroughWalkthroughEagerly initialize and cache FlowController and RuleManager in DefaultMainComponents, change ResolverMap visibility to private final, add jakarta.annotation-api (v2.1.1) to core/pom.xml, and update docs/ROADMAP.md with two roadmap entries. Changes
Sequence Diagram(s)(omitted — changes are refactoring/dependency/docs only) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-12-19T09:01:49.730ZApplied to files:
📚 Learning: 2025-09-22T19:49:29.473ZApplied to files:
📚 Learning: 2025-08-22T13:18:09.463ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @core/pom.xml:
- Around line 400-405: Replace the javax.annotation dependency with the Jakarta
equivalent: remove the dependency entry for groupId "javax.annotation"
artifactId "javax.annotation-api" version "1.3.2" and add the dependency for
groupId "jakarta.annotation" artifactId "jakarta.annotation-api" version "2.1.1"
so the project uses the Jakarta namespace required by Spring Framework 6.2.15
and Java 21.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (1)
94-108:setRuleManager()is now broken with the final field.The
ruleManagerfield isfinaland initialized in the constructor, butsetRuleManager()still exists and registers a different instance to the registry. SincegetRuleManager()now returns the field directly (line 95), anyRuleManagerpassed to the setter will be ignored—creating a silent inconsistency between the registry and what's actually used.Either:
- Remove
setRuleManager()if it's no longer needed with eager initialization- Make
ruleManagernon-final and update the field in the setter- Throw an exception in
setRuleManager()if called after constructionOption 1: Remove the setter if no longer needed
- public void setRuleManager(RuleManager ruleManager) { - log.debug("Setting ruleManager."); - ruleManager.setRouter(router); - getRegistry().register("ruleManager", ruleManager); - }Option 2: Make field non-final and update in setter
- private final RuleManager ruleManager; + private RuleManager ruleManager;public void setRuleManager(RuleManager ruleManager) { log.debug("Setting ruleManager."); ruleManager.setRouter(router); + this.ruleManager = ruleManager; getRegistry().register("ruleManager", ruleManager); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/pom.xmlcore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javadocs/ROADMAP.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T09:01:49.730Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2441
File: annot/pom.xml:0-0
Timestamp: 2025-12-19T09:01:49.730Z
Learning: In the annot module (annot/pom.xml), log4j dependencies (log4j-slf4j2-impl and log4j-core) should have <scope>test</scope>, while in the distribution module they should be normal compile-scope dependencies.
Applied to files:
core/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
docs/ROADMAP.md (1)
25-27: LGTM!The roadmap entries for
@PostConstructlifecycle management andbeanRegistryexposure are consistent with the dependency and component wiring changes in this PR.core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (2)
50-66: LGTM on eager initialization approach.The shift to eagerly initializing
flowControllerandruleManagerin the constructor with proper router wiring improves consistency and thread-safety. This addresses the registry sync issue by ensuring these components exist before any registry lookups.
169-171: LGTM!Returning the pre-initialized
flowControlleris consistent with the new eager initialization pattern, and there's no setter to create a mismatch.
|
This pull request needs "/ok-to-test" from an authorized committer. |
Summary by CodeRabbit
Chores
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.