split BeanRegistry interface#2488
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors the bean/YAML subsystem: moves types into a new Changes
Sequence Diagram(s)sequenceDiagram
participant K8s as KubernetesWatcher
participant Async as AsyncBeanCollector
participant Registry as BeanRegistryImplementation
participant Observer as BeanCacheObserver
K8s->>Async: create4Kubernetes(event) + isLast flag
Async->>Async: enqueue ChangeEvent
Note right of Async `#DDDDFF`: background virtual thread processes queue
Async->>Registry: handle(ChangeEvent, isLast)
Registry->>Registry: update BeanContainer map / activation queue
Registry->>Observer: handleBeanEvent(BeanDefinitionChanged, bean, oldBean)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 5
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/beanregistry/BeanDefinition.java (1)
37-40: Null check forkind2is unnecessary.
JsonNode.asText()never returnsnull- it returns an empty string""if the node is missing or null. The checkif (kind2 == null)will never be true.🔎 Proposed fix
- var kind2 = node.get("kind").asText(); - if (kind2 == null) - kind2 = "api"; - kind = kind2; + var kind2 = node.get("kind"); + kind = (kind2 == null || kind2.isNull()) ? "api" : kind2.asText();Alternatively, if "kind" node is always expected to exist:
- var kind2 = node.get("kind").asText(); - if (kind2 == null) - kind2 = "api"; - kind = kind2; + kind = node.path("kind").asText("api");
🧹 Nitpick comments (6)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)
41-47: Method signature correctly updated to use BeanDefinitionChanged.The signature change aligns with the new event-driven architecture.
Optional: Consider renaming parameter for clarity
The parameter name
bdtraditionally suggests "bean definition" but now holds aBeanDefinitionChangedevent. Considereventorchangefor clarity:- void handleBeanEvent(BeanDefinitionChanged bd, Object bean, Object oldBean) throws IOException; + void handleBeanEvent(BeanDefinitionChanged event, Object bean, Object oldBean) throws IOException;However, if
bdis an established convention in the codebase, keeping it is fine.annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)
17-19: Unused import ofWatchAction.The
WatchActionimport and static import are not used in this file. They may be needed byBeanDefinitionChanged(defined in a separate file), but these imports should be removed fromChangeEvent.java.🔎 Proposed fix
package com.predic8.membrane.annot.beanregistry; -import com.predic8.membrane.annot.yaml.WatchAction; - -import static com.predic8.membrane.annot.yaml.WatchAction.*; - public sealed interface ChangeEvent permits BeanDefinitionChanged, StaticConfigurationLoaded { }annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (3)
140-142:getFirstByNamesearches by name butbcsis keyed by UID.This method iterates over all values to find a bean by name, which is O(n). If
resolveReferenceis called frequently, consider maintaining an additional index (e.g.,Map<String, BeanContainer>by name) for O(1) lookups.
64-66: Emptystart()method - verify if intentional.The
start()method is empty. If this is intentional (lifecycle handled elsewhere), consider adding a brief comment explaining why, or remove the override if not needed by the interface contract.
68-86: Document or enforce single-threaded access tohandle()methodThe
handle()method modifiesuidsToActivate(aLinkedHashSet) without synchronization. WhilebcsandsingletonBeansuseConcurrentHashMap, this inconsistency creates a race condition risk ifhandle()is called concurrently. In production (Kubernetes pattern),AsyncBeanCollectorprovides protection by serializing calls through a single worker thread. However, direct usage ofBeanRegistryImplementationwith concurrent event sources would be unsafe. Either synchronize access touidsToActivate, convert it to a thread-safe collection, or document that this class must be used either throughAsyncBeanCollectoror with guaranteed single-threaded access tohandle().annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
19-19: Unused static import.The static import
import static com.predic8.membrane.annot.yaml.WatchAction.*;is not used in this file. TheWatchActiontype import on line 17 is sufficient for thecreate4Kubernetesmethod signature.🔎 Proposed fix
import com.predic8.membrane.annot.yaml.WatchAction; -import static com.predic8.membrane.annot.yaml.WatchAction.*; - public class BeanDefinition {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
annot/pom.xml(1 hunks)annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java(3 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(0 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(3 hunks)core/src/main/java/com/predic8/membrane/core/Router.java(3 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(2 hunks)core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(4 hunks)core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-19T09:01:40.311Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2441
File: annot/pom.xml:0-0
Timestamp: 2025-12-19T09:01:40.311Z
Learning: In annot/pom.xml, ensure log4j dependencies log4j-slf4j2-impl and log4j-core have <scope>test</scope>. In the distribution module (separate pom.xml), these dependencies should use the normal compile-scope. This guidance applies to all pom.xml files under the annot and distribution module structure where these dependencies appear; verify both modules are configured accordingly to reflect testing vs. runtime usage.
Applied to files:
annot/pom.xml
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java
🧬 Code graph analysis (6)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
BeanDefinition(21-103)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (3)
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (1)
BeanFactory(31-252)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(40-299)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
BeanContainer(5-28)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
BeanDefinition(21-103)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
BeanRegistryImplementation(28-165)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(40-299)
core/src/main/java/com/predic8/membrane/core/Router.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
BeanDefinition(21-103)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
BeanRegistryImplementation(28-165)
⏰ 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 (32)
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)
114-114: LGTM! Package reference updated correctly.The string literal correctly reflects the new package location for BeanRegistry.
annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
17-17: LGTM! Import updated for package refactoring.The import correctly reflects the relocated BeanRegistry interface.
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)
18-18: LGTM! Import updated for package refactoring.The import correctly reflects the relocated BeanRegistry interface.
core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java (1)
27-27: LGTM! Import updated for package refactoring.The import correctly reflects the relocated BeanRegistry interface. Note that this test is currently disabled (Line 40) with a TODO to rewrite to new parsing.
annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (1)
17-17: LGTM! Import updated for package refactoring.The import correctly reflects the relocated BeanRegistry interface.
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
16-16: LGTM! Improved import specificity.Replacing the wildcard import with a specific import for BeanRegistry is a good practice that improves code clarity and aligns with the package refactoring.
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
18-19: LGTM! Imports updated for package refactoring.Both imports correctly reflect the new bean registry architecture:
- BeanDefinition is now a separate class in the beanregistry package
- BeanRegistry has been relocated to the beanregistry package
These changes align with the TestRegistry implementation (line 346) which uses BeanDefinition in its method signature.
annot/pom.xml (1)
59-63: SpotBugs annotations version 4.9.8 is current and has no known security vulnerabilities.The version specified is the latest available release and is appropriate for use.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)
14-28: LGTM! Clean interface simplification.The package relocation and API reduction align well with the PR objectives. The interface now has a focused contract with just the essential methods.
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (1)
18-18: LGTM! Import correctly updated.The import path update aligns with the BeanRegistry relocation to the beanregistry package.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)
17-20: LGTM! Imports correctly added.The new imports support the updated event model using BeanDefinitionChanged.
core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java (1)
19-19: LGTM! Import correctly updated.The import path reflects the BeanRegistry relocation to the beanregistry package.
annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java (1)
17-30: LGTM! Excellent addition of semantic predicates.The predicate methods (
isDeleted(),isModified(),isAdded()) improve code readability and provide a cleaner API compared to direct enum comparisons.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java (1)
1-11: LGTM! Clean event wrapper design.The record-based implementation is appropriate for this immutable event type, and the design clearly separates the action from the definition data.
annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (1)
18-18: LGTM! Import correctly updated.The import update aligns with the BeanRegistry relocation, and the test logic remains unchanged.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
5-28: LGTM! Simple container class with appropriate visibility guarantees.The
volatilemodifier onsingletonensures proper visibility across threads. The class serves as a straightforward container for bean lifecycle management.core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (4)
17-21: LGTM!The imports are correctly updated to use the new
beanregistrypackage structure, reflecting the refactored architecture.
41-52: LGTM!The refactoring properly separates grammar from the bean registry and introduces async event handling via
AsyncBeanCollector. The initialization order is correct: grammar first, then the synchronous registry, then the async wrapper.
64-64: LGTM!Direct access to
grammar.getCrdSingularNames()is cleaner than the previous indirection through the bean registry.
107-117: VerifyisLast=truefor every Kubernetes event.Each Kubernetes watch event passes
isLast=truetohandle(). This is likely intentional since K8s events arrive individually and should trigger immediate processing, but confirm this aligns with the expected batch semantics ofAsyncBeanCollector.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
94-116: LGTM!The refactoring to return
List<BeanDefinition>instead of aBeanRegistrycleanly separates parsing from registry management. This enables the new event-driven architecture where callers can emit change events for each definition.
23-25: Remove unused importBeanRegistryImplementation.
BeanRegistryImplementation(line 25) is not used in this file. WhileBeanRegistry(line 24) is used as a parameter type on line 147 and should be kept,BeanRegistryImplementationcan be safely removed.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/Router.java (2)
18-19: LGTM!Imports correctly added for the new event types from the
beanregistrypackage.
654-677: LGTM!The method correctly adapts to the new
BeanDefinitionChangedevent type. Access patterns viabdc.bd()for the bean definition andbdc.action().isAdded()/isDeleted()/isModified()for action predicates are clean and readable.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java (2)
22-29: LGTM!The
parseYamlsdefault method provides a clean convenience flow:
- Parse YAML to bean definitions
- Emit
ADDEDevents with properisLasttracking- Emit
StaticConfigurationLoadedas the final event- Call
start()to begin processingThe
isLastflag correctly identifies the lastBeanDefinitionChangedevent in the loop, andStaticConfigurationLoadedis appropriately marked as the final event before starting.
31-40: LGTM!The abstract method declarations establish a clear contract. The
isLastparameter documentation is helpful for implementers to understand batch completion semantics.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)
51-53: LGTM! Clean two-step initialization pattern.The refactored initialization correctly creates the
BeanRegistryImplementation, callsparseYamlson it, and then assigns it to thebeanRegistryfield. This aligns with the new collector-based architecture.
85-85: Callback signature correctly updated.The
handleBeanEventmethod signature now acceptsBeanDefinitionChangedinstead ofBeanDefinition, aligning with the new event-driven model.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
49-62: LGTM! Clean bean instantiation logic.The
definemethod correctly distinguishes between "bean" kind (usingBeanFactory) and other kinds (usingGenericYamlParser), with proper exception handling.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
48-50: LGTM! Clean factory method for Kubernetes integration.The
create4Kubernetesfactory method properly encapsulates the creation of aBeanDefinitionChangedevent from aWatchActionandJsonNode, maintaining separation of concerns.core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
172-173: The code is correctly implemented. TheparseYamlsmethod is a default method in theBeanCollectorinterface that handles the complete lifecycle internally: it parses the YAML, processes the bean definitions, and callsstart()at the end (BeanCollector.java, lines 22-28). The variable does not need to be used after the call, and no explicitstart()invocation is required—it's already handled byparseYamls.annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)
28-28: No changes needed. The package-private visibility ofStaticConfigurationLoadedis correct and follows the sealed class design pattern, where permitted subclasses need not be public even when implementing a public sealed interface. The record is not used externally.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java (2)
31-39: Considerwhile (!Thread.currentThread().isInterrupted())and restoring interrupt flag.The loop condition
while (true)works butwhile (!Thread.currentThread().isInterrupted())is more idiomatic. Also, when catchingInterruptedException, best practice is to restore the interrupt flag before breaking.🔎 Proposed improvement
t = Thread.ofVirtual().start(() -> { - while (true) { + while (!Thread.currentThread().isInterrupted()) { try { ChangeEvent changeEvent = changeEvents.take(); delegate.handle(changeEvent, changeEvents.isEmpty()); } catch (InterruptedException e) { + Thread.currentThread().interrupt(); break; } } });
10-41: Missingstop()method for graceful shutdown.There's no way to stop the background thread. Consider adding a
stop()method to interrupt the thread and allow clean shutdown, especially important for testing and application lifecycle management.🔎 Proposed addition
public synchronized void stop() { if (t != null) { t.interrupt(); t = null; } }annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
59-61: Generic exception catch loses context.Catching
Exceptionand wrapping in a bareRuntimeExceptiondiscards the original exception type and any semantic meaning. Consider using a more specific exception or preserving context.🔎 Proposed improvement
} catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to define bean: " + bd.getUid(), e); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (4)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
Grammar(33-190)annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (1)
BeanFactory(31-252)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(40-299)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
BeanContainer(5-28)
⏰ 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 (3)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (3)
106-110: Exception during activation aborts remaining beans.When one bean fails to activate, the exception is logged and re-thrown, which stops processing all remaining beans in
uidsToActivate. Consider whether partial activation should be allowed, or at minimum ensureuidsToRemoveis applied before rethrowing.Currently, successfully processed beans (in
uidsToRemove) are never removed fromuidsToActivateif a later bean fails. This could lead to duplicate processing on retry.
64-66: Emptystart()method - intentional placeholder?The
start()method is empty. If this is intentional (e.g., this implementation doesn't need startup logic), consider adding a brief comment explaining this. Otherwise, ensure any required initialization is added.
112-113: Previous type mismatch bug is fixed.The removal loop now correctly iterates over
UidActionobjects and removes them from theSet<UidAction>, addressing the previous review concern about type mismatch.
3 participants
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.