Skip to content

Conversation

@christiangoerdes
Copy link
Collaborator

@christiangoerdes christiangoerdes commented Dec 22, 2025

Summary by CodeRabbit

  • New Features

    • Added asynchronous bean collection support to handle configuration changes with improved performance and responsiveness.
  • Chores

    • Refactored internal bean registry architecture to improve modularity and separation of concerns.
    • Reorganized bean management components for better maintainability and clarity.
    • Enhanced bean lifecycle handling and activation mechanisms for more efficient resource management.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Walkthrough

A substantial refactoring restructures the bean registry and YAML parsing subsystem. Bean registry classes move to a new package; APIs are renamed and simplified; a new async event-handling layer is introduced via AsyncBeanCollector; and BeanDefinitionChanged replaces action tracking embedded in BeanDefinition. The bean initialization flow transitions from explicit registerBeanDefinitions() and start() calls to a unified parseYamls() method. Integration points across the codebase are updated accordingly.

Changes

Cohort / File(s) Summary
Bean Registry Core Interfaces & Package Move
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java, ChangeEvent.java
BeanRegistry and ChangeEvent moved from yaml to beanregistry package. Method resolveReference(String) renamed to resolve(String). Method getBeans() generified to getBeans(Class<T> clazz). Methods registerBeanDefinitions() and start() removed. ChangeEvent promoted from package-private sealed interface to public.
New Bean Definition & Container Classes
BeanDefinition.java, BeanDefinitionChanged.java, BeanContainer.java
BeanDefinition moved to beanregistry package, simplified to hold only JsonNode; constructor now private with single JsonNode parameter. Action-state tracking removed; methods getAction(), getBean(), setBean(), isDeleted(), isModified(), isAdded() removed. Factory method create4Kubernetes() now returns BeanDefinitionChanged instead of BeanDefinition. New BeanDefinitionChanged record (public, with action and bd fields). New BeanContainer class holds definition + thread-safe singleton instance.
New Collector & Registry Interfaces
BeanCollector.java
New public interface BeanCollector defining lifecycle: parseYamls(InputStream, Grammar) (default impl), handle(ChangeEvent, boolean isLast), and start(). Serves as configuration ingestion contract.
Async Event Handler
AsyncBeanCollector.java
New public class wrapping BeanCollector, buffers ChangeEvent in LinkedBlockingDeque, processes asynchronously in dedicated thread, delegates to wrapped collector via handle(). Synchronized start() method initializes and starts background thread.
New Registry Implementation
BeanRegistryImplementation.java (beanregistry)
New implementation coordinating BeanRegistry and BeanCollector. Manages bean definitions, activation queuing, and lifecycle. On StaticConfigurationLoaded, triggers activation run; on BeanDefinitionChanged, updates containers and queues activations. Provides resolution, bean retrieval, and grammar access. Replaces old yaml-package BeanRegistryImplementation.
Core Factory & Observer Updates
BeanFactory.java, BeanCacheObserver.java
BeanFactory imports updated; method call changed from registry.resolveReference() to registry.resolve(). BeanCacheObserver interface parameter type changed from BeanDefinition to BeanDefinitionChanged in handleBeanEvent() signature.
YAML Parsing & Reference Resolution
GenericYamlParser.java, MethodSetter.java, ParsingContext.java
Imports updated to reflect beanregistry package moves. Reference resolution calls updated from resolveReference() to resolve(). Documentation clarified. ParsingContext adds beanregistry import.
Watch Action Helpers
WatchAction.java
Three new public helper methods added: isDeleted(), isModified(), isAdded() for enum-value predicate checks.
Removed Old Implementation
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
Entire file removed; old multithreaded implementation with bean definition caching, activation queue, and change-event handling no longer present in yaml package.
Bean Registry Initialization Flow
YamlParser.java (test util), RouterCLI.java
Initialization refactored from two-step (registerBeanDefinitions() + start()) to single-step (parseYamls()). Local variable type changes from BeanRegistry to BeanCollector. BeanCacheObserver callback parameter updated to BeanDefinitionChanged.
Integration Updates — Core
Router.java, KubernetesWatcher.java, Envelope.java
Router.handleBeanEvent() parameter updated to BeanDefinitionChanged, control flow refactored to use bdc.action().isAdded() etc. KubernetesWatcher introduces AsyncBeanCollector wrapper, wraps BeanRegistryImplementation for async event handling. Imports updated throughout for package moves.
Import Path Updates & Test Adjustments
YAMLBeanParsingTest.java, YAMLComponentsParsingTest.java, CompilerHelper.java, InMemoryClassLoader.java, StructureAssertionUtil.java, EnvelopeTest.java, GenericYamlParserTest.java
Import paths updated from com.predic8.membrane.annot.yaml.BeanRegistry to com.predic8.membrane.annot.beanregistry.BeanRegistry. Test overrides and class-loader delegation updated to match new package location. API calls (resolveReference()resolve()) updated in test code.
Dependency Addition
pom.xml
Added dependency: com.github.spotbugs:spotbugs-annotations:4.9.8.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Areas requiring extra attention:

  • BeanRegistryImplementation.java (new, beanregistry package): Complex state management (bean containers, activation queues, change event handling, lifecycle coordination between BeanRegistry and BeanCollector interfaces). Verify thread-safety, activation order, and observer notification consistency.
  • BeanDefinition.java (package move + API changes): Verify all call sites of removed methods (getAction(), getBean(), setBean(), isDeleted(), isModified(), isAdded()) have been updated; confirm factory method create4Kubernetes() return-type change is handled correctly.
  • BeanRegistry.java (interface changes): Method renaming (resolveReference()resolve()), removal of registerBeanDefinitions() and start(), generification of getBeans(). Ensure all implementations and callers are consistent.
  • KubernetesWatcher.java: New async wrapper (AsyncBeanCollector) integration; verify event handling, grammar initialization, and background thread lifecycle.
  • Router.java: Parameter and control-flow updates in handleBeanEvent(); ensure action checks (isAdded(), isDeleted(), isModified()) mirror original semantics.
  • Integration across multiple modules: Verify package moves are complete, no dangling old imports remain, and the new parseYamls() flow correctly replaces old two-step initialization.

Possibly related PRs

  • Add Yaml bean support #2436: Overlapping YAML bean registry refactoring including BeanRegistry API updates, BeanDefinitionChanged introduction, and resolveReference()resolve() rename.
  • Yaml bean registry #2339: Related "YAML bean registry" architectural refactor affecting BeanRegistry, BeanDefinition, GenericYamlParser, and BeanRegistryImplementation classes and APIs.
  • Fix load order of APIs #2413 #2414: Overlaps in bean registry lifecycle API changes (e.g., start(), registerBeanDefinitions(), parsing flow refactoring).

Suggested labels

7.x

Suggested reviewers

  • rrayst
  • predic8

Poem

🐰 Hoppy refactor, new paths align,
Beans now collect in async design,
With BeanDefinitionChanged so neat,
Registry moves make the pattern complete!
Old code departs, new structure reigns—
Watch and resolve through cleaner lanes. 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main API changes: renaming resolveReference to resolve and adding the generic getBeans method with type parameter.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch beanregistry-improvements

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@christiangoerdes
Copy link
Collaborator Author

Merge #2488 first

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (6)
annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (1)

70-84: LGTM!

Test correctly uses the renamed resolve method. The test logic for singleton verification is sound.

Consider renaming the test method from singletonResolveReference to singletonResolve to match the new API naming.

annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)

18-18: Consider explicit imports over wildcard.

Wildcard imports can obscure dependencies. Consider explicit imports for clarity:

import com.predic8.membrane.annot.beanregistry.BeanCacheObserver; import com.predic8.membrane.annot.beanregistry.BeanDefinition; import com.predic8.membrane.annot.beanregistry.BeanDefinitionChanged; import com.predic8.membrane.annot.beanregistry.BeanRegistry; import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)

17-19: Remove unused import.

BeanRegistryImplementation is imported but not used in this interface. Consider removing it to keep the imports clean.

🔎 Proposed fix
 import com.predic8.membrane.annot.beanregistry.BeanDefinition; import com.predic8.membrane.annot.beanregistry.BeanDefinitionChanged; -import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java (1)

22-25: Consider documenting that isLast is intentionally ignored.

The isLast parameter is ignored because the async wrapper recalculates it based on queue state. A brief comment would clarify this design decision.

 @Override public void handle(ChangeEvent changeEvent, boolean isLast) { + // isLast is intentionally ignored; recalculated based on queue state in worker thread changeEvents.add(changeEvent); }
annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)

17-19: Unused imports.

WatchAction and its static import are not used in this file. They may be leftover from refactoring or intended for BeanDefinitionChanged which is defined in a separate file.

🔎 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 {
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)

17-19: Unused import: BeanRegistry.

BeanRegistry is imported but not used in this file. Only BeanCollector and BeanRegistryImplementation are referenced.

🔎 Proposed fix
 import com.predic8.membrane.annot.beanregistry.BeanCollector; -import com.predic8.membrane.annot.beanregistry.BeanRegistry; import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f05e64e and d82b45b.

📒 Files selected for processing (28)
  • annot/pom.xml
  • annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java
  • annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java
  • annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
  • annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java
  • annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java
  • annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java
  • annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java
  • core/src/main/java/com/predic8/membrane/core/Router.java
  • core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
  • core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java
  • core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java
  • core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java
  • core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
💤 Files with no reviewable changes (1)
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
🧰 Additional context used
🧠 Learnings (3)
📚 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
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8 Repo: membrane/api-gateway PR: 2240 File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215 Timestamp: 2025-10-23T13:04:11.388Z Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision. 

Applied to files:

  • annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java
🧬 Code graph analysis (5)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
  • McYamlIntrospector (28-185)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.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/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 (29-176)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.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 (29-176)
⏰ 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 (45)
core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java (1)

27-27: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)

17-17: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)

18-18: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java (1)

19-19: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)

16-16: LGTM! Import improved from wildcard to specific import.

The change from wildcard import to specific import is better practice and correctly reflects the BeanRegistry package relocation.

annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (1)

18-18: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)

107-107: LGTM! API method renamed from resolveReference to resolve.

The call has been correctly updated to use the renamed method. This change aligns with the PR objectives to simplify the BeanRegistry API.


130-146: LGTM! API method renamed from resolveReference to resolve.

The call at Line 134 has been correctly updated to use the renamed method. The error handling and type checking logic remain unchanged.

annot/pom.xml (1)

59-63: Approved. SpotBugs annotations added for static analysis support.

Version 4.9.8 is current and officially released. The compile scope is correct for annotations used in code documentation.

annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)

111-115: LGTM!

The delegation check correctly references the new package location for BeanRegistry (com.predic8.membrane.annot.beanregistry.BeanRegistry), ensuring the test class loader delegates to the parent for this interface.

annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (2)

18-18: LGTM!

Import correctly references the relocated BeanRegistry interface.


240-251: LGTM!

The method rename from resolveReference to resolve is applied correctly. The null-safety check and type-compatibility validation logic remain intact.

core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (2)

18-19: LGTM!

Imports correctly updated to reference BeanDefinition and BeanRegistry from the new beanregistry package.


335-359: LGTM!

TestRegistry correctly implements the renamed resolve method. The stub implementations for other BeanRegistry methods are appropriate for these parameterized tests that only exercise reference resolution.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java (1)

1-11: LGTM!

Well-designed record for signaling bean definition changes. The pairing of WatchAction and BeanDefinition clearly captures the event semantics.

Consider adding null validation if action or bd should never be null at construction time:

public record BeanDefinitionChanged( WatchAction action, BeanDefinition bd) implements ChangeEvent { public BeanDefinitionChanged { Objects.requireNonNull(action, "action must not be null"); Objects.requireNonNull(bd, "bd must not be null"); } }
annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java (1)

16-30: LGTM!

The predicate methods (isDeleted(), isModified(), isAdded()) provide a more readable API at call sites compared to direct enum comparisons.

annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (3)

17-17: LGTM!

Import correctly updated to the new beanregistry package.


86-100: LGTM!

Test correctly uses the renamed resolve method for prototype scope verification.


102-113: LGTM!

Error handling test correctly uses the renamed resolve method.

annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (4)

23-24: LGTM!

Imports correctly reference BeanDefinition and BeanRegistry from the new beanregistry package.


92-103: LGTM!

Javadoc accurately updated to reflect that the method returns "list of parsed bean definitions" rather than a fully populated registry.


262-268: LGTM!

The resolve method call correctly replaces resolveReference, with existing error handling preserved.


293-297: LGTM!

The $ref handling path correctly uses the renamed resolve method.

annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)

51-56: LGTM!

The two-step initialization pattern is correctly implemented:

  1. Create BeanRegistryImplementation with observer and grammar
  2. Parse YAMLs via parseYamls
  3. Assign to the beanRegistry field (typed as BeanRegistry interface)

This allows the implementation-specific parseYamls method to be called before treating the registry through its interface.


84-88: LGTM!

The handleBeanEvent signature correctly updated to accept BeanDefinitionChanged bdc instead of BeanDefinition bd, aligning with the new event model that pairs action with definition.

core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (2)

107-117: Consider whether isLast=true is always appropriate for watch events.

The handle method is always called with isLast=true. This works correctly for individual Kubernetes watch events processed one at a time. If batch processing is ever introduced, this would need adjustment to properly signal the last event in a batch.


49-51: No action required. Router implements BeanCacheObserver, so the constructor call is type-safe and correct. The code at lines 49-51 passes router to BeanRegistryImplementation, which expects a BeanCacheObserver as its first parameter—this is valid.

Likely an incorrect or invalid review comment.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java (2)

22-29: LGTM!

The parseYamls default method correctly orchestrates YAML ingestion: parsing, emitting BeanDefinitionChanged events with proper isLast flags, signaling StaticConfigurationLoaded, and finally calling start(). The empty list case is handled correctly since StaticConfigurationLoaded is always emitted.


31-41: Clean interface design.

The abstract methods handle and start are well-documented and provide a clear contract for implementations. The separation between event handling and lifecycle management is appropriate.

annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)

47-47: API change from BeanDefinition to BeanDefinitionChanged is appropriate.

Using BeanDefinitionChanged provides the WatchAction context (ADDED/MODIFIED/DELETED) alongside the BeanDefinition, which is more informative for event handling. This aligns with the new event-driven architecture.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)

3-26: LGTM - simple and clean container class.

The design is straightforward. The volatile modifier on singleton ensures visibility across threads. If singleton initialization is performed in a single-threaded context (e.g., during bean registry startup) with only concurrent reads afterward, this is appropriate.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)

20-28: Clean API refinement.

The changes improve the interface:

  • resolve is more concise than resolveReference while maintaining clarity.
  • The generic getBeans(Class<T> clazz) enables type-safe retrieval, complementing the existing getBeans() for unfiltered access.
  • Separation of concerns is improved by moving lifecycle methods (registerBeanDefinitions, start) to BeanCollector.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)

21-28: LGTM!

The sealed interface design cleanly separates configuration events. StaticConfigurationLoaded being package-private appropriately restricts its creation to internal components.

core/src/main/java/com/predic8/membrane/core/Router.java (2)

18-19: LGTM!

The imports correctly reflect the refactored types: BeanDefinition for the isActivatable method and BeanDefinitionChanged for the event-driven handleBeanEvent method.


654-677: LGTM!

The refactored method correctly uses BeanDefinitionChanged to encapsulate both the action type and bean definition. The pattern bdc.action().isAdded() cleanly separates concerns, and accessing the wrapped definition via bdc.bd() is straightforward.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (6)

50-63: LGTM!

The define method correctly dispatches between BeanFactory for explicit "bean" kinds and GenericYamlParser for other types. Exception wrapping is appropriate for propagation.


69-87: LGTM!

The handle method cleanly uses pattern matching for deconstruction. The flow correctly processes StaticConfigurationLoaded and BeanDefinitionChanged events, triggering activation when appropriate.


124-139: LGTM!

The resolve method correctly handles prototype vs singleton scope: prototypes are always instantiated fresh, while singletons are cached in the container. The exception message is clear when a reference is not found.


145-161: Inconsistent filtering between getBeans() and getBeans(Class<T>).

getBeans() filters out components via !bd.getDefinition().isComponent(), but getBeans(Class<T>) does not apply this filter. This may return component beans unexpectedly when filtering by class.

Verify if this inconsistency is intentional. If not:

🔎 Proposed fix to add component filtering
 @Override public <T> List<T> getBeans(Class<T> clazz) { return bcs.values().stream() + .filter(bd -> !bd.getDefinition().isComponent()) .map(BeanContainer::getSingleton) .filter(Objects::nonNull) .filter(clazz::isInstance) .map(clazz::cast) .toList(); }

168-175: LGTM!

The prototype scope detection correctly differentiates between regular definitions (using isPrototype()) and explicit "bean" kinds (reading from JSON). Defaulting to SINGLETON is a sensible convention.


37-41: Thread-safety of uidsToActivate requires either explicit documentation or synchronization.

In production usage (KubernetesWatcher), BeanRegistryImplementation is wrapped with AsyncBeanCollector, which enforces thread confinement by processing all handle() calls from a single background thread. However, the code doesn't document this assumption, and direct usage without AsyncBeanCollector (as in RouterCLI and tests) could allow concurrent modification of the plain LinkedHashSet if called from multiple threads.

Consider either: (1) documenting that handle() must be called from a single thread, or (2) making uidsToActivate thread-safe (e.g., Collections.synchronizedSet() or adding synchronization) to prevent accidental misuse when the wrapper is omitted.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (3)

14-19: LGTM!

Package relocation to beanregistry and the WatchAction import support the new factory method pattern.


34-50: LGTM!

The refactoring cleanly separates concerns: the private constructor handles K8S-specific JSON parsing, while create4Kubernetes returns a BeanDefinitionChanged that encapsulates both the action and definition. This aligns well with the event-driven architecture introduced in this PR.


52-102: LGTM!

The public constructor and accessor methods remain intact for non-Kubernetes use cases. The utility methods (isComponent, isBean, isPrototype) provide clean abstractions for scope and type determination.

core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)

172-173: Review parseYamls method signature for potential simplification.

The grammar parameter passed to parseYamls duplicates the instance field stored during construction. Both current usages pass identical grammar instances to the constructor and method. Consider whether parseYamls should use the stored instance grammar instead of requiring the parameter, or document if allowing different grammars per parse operation is intentional design.

Comment on lines +31 to +42
t = new Thread(() -> {
while (true) {
try {
ChangeEvent changeEvent = changeEvents.take();
delegate.handle(changeEvent, changeEvents.isEmpty());
} catch (InterruptedException e) {
break;
}
}
});
t.start();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Thread lifecycle and configuration issues.

Several concerns with the background thread:

  1. No shutdown mechanism: There's no stop() method to gracefully terminate the thread. Consider adding one that interrupts the thread and awaits termination.

  2. Non-daemon thread: The thread will prevent JVM shutdown. Consider setting t.setDaemon(true) or providing explicit shutdown.

  3. Thread naming: Add a descriptive name for debugging: t.setName("AsyncBeanCollector-worker").

  4. Exception handling: If delegate.handle() throws an unchecked exception, the thread will terminate silently.

🔎 Proposed improvements
 @Override public synchronized void start() { if (t != null) return; t = new Thread(() -> { while (true) { try { ChangeEvent changeEvent = changeEvents.take(); delegate.handle(changeEvent, changeEvents.isEmpty()); } catch (InterruptedException e) { break; + } catch (Exception e) { + // Log and continue processing + e.printStackTrace(); } } }); + t.setName("AsyncBeanCollector-worker"); + t.setDaemon(true); t.start(); } + + public synchronized void stop() { + if (t != null) { + t.interrupt(); + try { + t.join(5000); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } + t = null; + } + }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java around lines 31-42, the background thread lacks lifecycle controls and resilience: add a public stop() method that interrupts the worker thread and waits (join with a timeout) for it to terminate, set the thread to daemon (t.setDaemon(true)) and give it a descriptive name (t.setName("AsyncBeanCollector-worker")), and wrap delegate.handle(...) in a try/catch(Throwable) that logs unexpected runtime errors and continues the loop so the thread does not die silently; ensure InterruptedException handling breaks the loop and preserves interrupt status as appropriate. 
Comment on lines +113 to +114
for (String uid : uidsToRemove)
uidsToActivate.remove(uid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: Removal from uidsToActivate will never work.

uidsToActivate is a Set<UidAction>, but the removal loop attempts to remove String UIDs. Since UidAction is a record with (String uid, WatchAction action), calling uidsToActivate.remove(uid) where uid is a String will never match any UidAction element.

🔎 Proposed fix
- for (String uid : uidsToRemove) - uidsToActivate.remove(uid); + uidsToActivate.removeIf(ua -> uidsToRemove.contains(ua.uid()));

Or alternatively, clear processed items inline:

 private void activationRun() { - Set<String> uidsToRemove = new HashSet<>(); + Iterator<UidAction> it = uidsToActivate.iterator(); - for (UidAction uidAction : uidsToActivate) { + while (it.hasNext()) { + UidAction uidAction = it.next(); BeanContainer bc = bcs.get(uidAction.uid()); try { // ... existing code ... - uidsToRemove.add(bc.getDefinition().getUid()); + it.remove(); } catch (Exception e) { // ... existing code ... } } - for (String uid : uidsToRemove) - uidsToActivate.remove(uid); }
🤖 Prompt for AI Agents
In annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java around lines 113-114, the loop calls uidsToActivate.remove(uid) using a String uid while uidsToActivate is a Set<UidAction>, so removals never match; replace the removal logic to remove the UidAction entries whose uid field equals the String (for example, use uidsToActivate.removeIf(ua -> uidsToRemove.contains(ua.uid())) or iterate with an Iterator and remove when ua.uid().equals(uid)), or simply clear processed items inline after mapping, ensuring type-correct removal of UidAction instances rather than attempting to remove Strings. 
@membrane-ci-server
Copy link

This pull request needs "/ok-to-test" from an authorized committer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants