Readme yaml snippets #2415
Readme yaml snippets #2415
Conversation
- Added `start()` method to `BeanRegistry` interface and its implementation for lifecycle management. - Documented thread-safety expectations and constraints for `registerBeanDefinitions()` and `start()`. - Updated usage in relevant classes to adhere to the modified lifecycle flow.
…amples for consistency and clarity
WalkthroughAdded lifecycle methods to the BeanRegistry API— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-23T15:59:51.742ZApplied 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). (2)
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 |
…emove redundant YAML schema reference
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
113-129: Thestart()method exits prematurely in Kubernetes mode, leaving enqueued events unconsumed.The current implementation calls
beanRegistry.start()once during Router startup (Router.java:304) on the main thread. The method processes initial events and exits when the queue is empty (while (!changeEvents.isEmpty())). However, Kubernetes watchers are created afterstart()returns and asynchronously add events viabeanRegistry.handle(). These events accumulate in the queue with no consumer running to process them.The Javadoc explicitly states: "In Kubernetes mode, run this method in a dedicated long-running thread to consume update events as they arrive," but the implementation contradicts this. Using a poison pill or shutdown signal instead of queue emptiness as the termination condition would allow
start()to block indefinitely when running in a dedicated consumer thread, properly handling continuous Kubernetes events.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
159-161: Avoid instantiatingGrammarAutoGeneratedtwice.
GrammarAutoGeneratedis created twice on consecutive lines. Extract it to a local variable to avoid redundant instantiation and ensure consistency.private static Router initRouterByYAML(String location) throws Exception { var router = new HttpRouter(); router.setBaseLocation(location); router.setHotDeploy(false); router.setAsynchronousInitialization(true); router.start(); - BeanRegistry registry = new BeanRegistryImplementation(router, new GrammarAutoGenerated()); - registry.registerBeanDefinitions(parseMembraneResources(router.getResolverMap().resolve(location), new GrammarAutoGenerated())); + var grammar = new GrammarAutoGenerated(); + BeanRegistry registry = new BeanRegistryImplementation(router, grammar); + registry.registerBeanDefinitions(parseMembraneResources(router.getResolverMap().resolve(location), grammar)); registry.start(); return router; }annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
201-202: Inefficient removal from List.Removing elements from an
ArrayListviaremove(Object)in a loop is O(n) per removal, resulting in O(n²) overall. For small lists this is acceptable, but if the activation list grows large, consider usingremoveAll(uidsToRemove)or switching to a data structure that supports O(1) removal.- for (String uid : uidsToRemove) - uidsToActivate.remove(uid); + uidsToActivate.removeAll(uidsToRemove);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(14 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(1 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-23T15:59:51.742Z
Learnt from: rrayst Repo: membrane/api-gateway PR: 2339 File: distribution/examples/web-services-soap/rest2soap-template/apis.yaml:17-32 Timestamp: 2025-11-23T15:59:51.742Z Learning: In Membrane API Gateway YAML configuration files, response flows execute in reverse order. Steps that appear later in the response flow list actually execute first. For example, if a template references properties and setProperty steps appear after the template in the YAML, those setProperty steps will execute before the template renders. Applied to files:
README.md
⏰ 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)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
348-351: LGTM!The empty
start()override correctly satisfies the updatedBeanRegistryinterface contract. For this test stub, no actual lifecycle activation is needed.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
53-53: LGTM!The
start()call correctly follows the new two-phase lifecycle pattern: firstregisterBeanDefinitions()to queue events, thenstart()to process and activate beans before awaiting completion.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (1)
26-79: LGTM!Excellent documentation for the new lifecycle methods. The Javadoc clearly explains thread-safety guarantees, lifecycle expectations, and usage constraints for both
registerBeanDefinitions()andstart().README.md (1)
731-744: Inconsistent root element: useapi:instead ofspec:.Same inconsistency as above—this example uses
spec:while the rest of the document usesapi:.-```yaml -spec: - port: 2000 - flow: +```yaml +api: + port: 2000 + flow:⛔ Skipped due to learnings
Learnt from: rrayst Repo: membrane/api-gateway PR: 2339 File: distribution/examples/web-services-soap/rest2soap-template/apis.yaml:17-32 Timestamp: 2025-11-23T15:59:51.742Z Learning: In Membrane API Gateway YAML configuration files, response flows execute in reverse order. Steps that appear later in the response flow list actually execute first. For example, if a template references properties and setProperty steps appear after the template in the YAML, those setProperty steps will execute before the template renders.
| private final Map<String, BeanDefinition> bds = new ConcurrentHashMap<>(); | ||
| private final Set<String> uidsToActivate = ConcurrentHashMap.newKeySet(); | ||
| private final Map<String, BeanDefinition> bds = new ConcurrentHashMap<>(); // Order is not critical. Order is determined by uidsToActivate | ||
| private final List<String> uidsToActivate = new ArrayList<>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BeanRegistryImplementation.java" 2>/dev/nullRepository: membrane/api-gateway
Length of output: 149
Duplicate UIDs possible in activation list when beans are modified after initial addition.
Changing from Set to List enables deterministic ordering but removes duplicate prevention. The notify() method routes both ADDED and MODIFIED events to handle() (line 188-190), which unconditionally appends UIDs to uidsToActivate (line 180) without checking if already present. If a bean is ADDED then MODIFIED, its UID will appear twice in the list, causing activate() to be called twice during start() (lines 218-219).
Use LinkedHashSet to maintain insertion order while preventing duplicates, or add a duplicate check: if (!uidsToActivate.contains(uid)) uidsToActivate.add(uid);
🤖 Prompt for AI Agents
In annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java around line 44, the uidsToActivate field is a List which allows duplicate UIDs when a bean is ADDED then MODIFIED (notify() routes both to handle() and handle() appends unconditionally), causing activate() to be invoked twice during start(); fix by changing uidsToActivate to a LinkedHashSet to preserve insertion order and prevent duplicates (or, if keeping a List, add a check before adding: only append if not already present), ensuring notify()/handle() logic can remain unchanged and start() will only activate each UID once. 
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.