Fix load order of APIs #2413#2414
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.
|
Caution Review failedThe pull request is closed. WalkthroughSeparates bean registration from activation by adding a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as RouterCLI
participant Parser as YamlParser
participant Registry as BeanRegistryImplementation
participant Router as Router
CLI->>Registry: new BeanRegistryImplementation(router, grammar)
CLI->>Parser: parseMembraneResources(...)
Parser->>Registry: registerBeanDefinitions(listOfBeanDefs)
Parser->>Registry: (returns)
CLI->>Registry: registry.start()
Registry->>Registry: process queued ADDED events (in insertion order)
Registry->>Router: activate beans / register components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
166-172: Potential duplicate UID entries inuidsToActivate.If a bean is first ADDED and then later MODIFIED before
activationRun()executes,uidsToActivate.add(bd.getUid())will insert the same UID twice. The removal loop at lines 201-202 usesList.remove(Object)which only removes the first occurrence, leaving duplicates behind.Consider checking for existing entries before adding, or use a
LinkedHashSetto maintain order while preventing duplicates:- private final List<String> uidsToActivate = new ArrayList<>(); + private final Set<String> uidsToActivate = new LinkedHashSet<>();Or guard the add:
if (observer.isActivatable(bd)) - uidsToActivate.add(bd.getUid()); + if (!uidsToActivate.contains(bd.getUid())) + uidsToActivate.add(bd.getUid());
113-129: Kubernetes watch events won't be processed;start()exits when queue empties and never consumes subsequent events.The
while (!changeEvents.isEmpty())loop exits immediately when the queue is empty, then returns. In Kubernetes mode, watchers add events asynchronously viabeanRegistry.handle()afterstart()has already returned. These queued events are never processed because no thread is consuming them.The javadoc explicitly states: "In Kubernetes mode, run this method in a dedicated long-running thread." However,
KubernetesWatcher.start()callsbeanRegistry.start()synchronously and doesn't create a background thread. The fix requires either:
- Removing the
isEmpty()check so the loop blocks ontake()indefinitely in a dedicated thread, or- Having
KubernetesWatchersubmitbeanRegistry.start()to an executor service that keeps it running.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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)
⏰ 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 (4)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (1)
26-79: Well-documented lifecycle contract.The Javadoc clearly explains the thread-safety model and lifecycle expectations for both methods. The separation of registration and activation phases is a sound design that enables order-preserving processing.
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
51-55: Correctly implements the new lifecycle pattern.The sequence
registerBeanDefinitions()→start()→await()properly follows the documented contract. Thestart()method processes events synchronously, so awaiting the latch afterward is correct.core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
348-351: Appropriate no-op implementation for test stub.The empty
start()method correctly satisfies the interface contract for this test registry, which only serves as a stub for bean resolution.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
44-44: Good: ArrayList preserves insertion order, fixing the load order issue.Switching from
Set<String>toList<String>correctly preserves the registration order of APIs, which is the core fix for issue #2413.
…dering and updated `GrammarAutoGenerated` handling in `RouterCLI`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
38-45: Deterministic activation order viauidsToActivatelooks good; consider guarding with testsUsing a
LinkedHashSetforuidsToActivatewhile keepingbdsas a concurrent map cleanly decouples storage from activation order and should fix the scrambled startup order, assuming producers enqueue in YAML order.I’d recommend adding/adjusting a focused test that registers multiple API
BeanDefinitions in a known sequence viaregisterBeanDefinitions(...)and asserts that the observer sees activation in exactly that sequence, to lock in this behavior.
51-80: Lifecycle split (registerBeanDefinitionsvsstart) is clearer; verify all callers now invokestart()The new Javadoc and behavior (register only, no implicit startup) make the registry lifecycle much clearer and avoid hidden side effects. One concern: any existing call sites that previously relied on
registerBeanDefinitions(...)implicitly triggering startup will now silently skip activation if they don’t callstart().Two suggestions:
- Double‑check all implementors/callers of
BeanRegistry(beyondYamlParserandRouterCLI) to ensure they now callstart()once after registration.- Optionally rename the parameter
bdsto something likedefinitionsto avoid shadowing/confusion with thebdsfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
⏰ 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)
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
…fety, and execution flow
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (1)
18-18: Consider using specific imports instead of wildcard.Wildcard imports can reduce code clarity and potentially lead to naming conflicts. Since this interface only needs
List, consider keeping the specific import.Apply this diff:
-import java.util.*; +import java.util.List;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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)
⏰ 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 (7)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (2)
26-49: Excellent documentation for the registration lifecycle.The Javadoc clearly documents thread-safety expectations, lifecycle requirements, and the relationship with
start(). This will help prevent misuse of the API.
50-76: Javadoc correctly describes single-pass startup behavior.The documentation has been updated to accurately reflect the implementation -
start()processes currently queued events and returns, rather than blocking as a long-running consumer. This addresses the previous review feedback effectively.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (5)
38-44: LinkedHashSet preserves insertion order—the core fix for load order issue.Switching
uidsToActivatetoLinkedHashSet(line 44) ensures thatactivationRun()processes beans in the order they were registered, directly addressing issue #2413 where API load order was scrambled. The clarifying comments on lines 38, 43, and 44 document which structures preserve order and which don't.Note on thread-safety:
LinkedHashSetis not thread-safe, but the Javadoc correctly documents that modification of this set occurs only within the single-threaded execution context ofstart()andhandle(BeanDefinition bd)(called fromstart()). The publichandle(WatchAction, ...)methods only enqueue to the thread-safeBlockingQueue, so the design is sound as long as callers honor the documented contract.
51-80: Registration implementation correctly defers activation.The implementation properly enqueues initialization events without performing activation work, as documented. The
StaticConfigurationLoadedmarker event signals the end of the registration phase, ensuringstart()can perform deterministic, ordered activation.
82-127: start() implementation correctly processes queued events in order.The implementation processes all currently queued events and then returns, matching the documented single-pass behavior. The
StaticConfigurationLoadedmarker triggersactivationRun()which iterates overuidsToActivatein the insertion order preserved byLinkedHashSet, ensuring deterministic startup sequencing.
140-170: Thread-safe event queueing with ordered processing.The public
handle(WatchAction, ...)methods (lines 140-150) safely enqueue events from multiple threads. The privatehandle(BeanDefinition bd)(lines 160-170) is invoked only bystart()in a single-threaded context, safely modifyinguidsToActivateand triggering immediate activation when the queue is empty.
172-201: Ordered activation preserves registration sequence.The iteration over
uidsToActivateat line 174 respects the insertion order maintained byLinkedHashSet, ensuring APIs are activated in the sequence they were registered. This directly solves the issue reported in #2413 where startup logs showed APIs starting in scrambled order (3, 1, 2) instead of the defined order (1, 2, 3).
2 participants
fixes #2413
Summary by CodeRabbit
Refactor
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.