Skip to content

Conversation

@robocanic
Copy link
Contributor

Please provide a description of this PR:
Introduce Informer Framwork from client-go to support ResourceEngine and ResourceDiscovery for list-watch.
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

  • Docs
  • Installation
  • User Experience
  • Dubboctl
  • Console
  • Core Component

Please check any characteristics that apply to this pull request.

@robocanic robocanic requested a review from Copilot August 24, 2025 09:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the Informer Framework from client-go to support ResourceEngine and ResourceDiscovery for list-watch functionality. It adds comprehensive event handling capabilities through an EventBus component and refactors the resource registration system to support better runtime object handling.

  • Implements an EventBus component for event distribution between components
  • Adds a comprehensive controller framework with Informer and ResourceListerWatcher interfaces
  • Refactors resource registration from kind-only to schema-based with factory functions

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/resourcegen/main.go Updates resource template generation to include DeepCopyObject methods and factory functions
pkg/core/resource/model/resource.go Adds k8s runtime.Object interface requirement to Resource interface
pkg/core/resource/model/registry.go Refactors from ResourceKindRegistry to ResourceSchemaRegistry with factory functions
pkg/core/events/eventbus.go Completely rewrites EventBus interface to support resource change events
pkg/core/events/component.go Implements EventBus as a runtime component with synchronous event processing
pkg/core/controller/*.go Adds new Informer framework for list-watch operations
pkg/core/engine/*.go Implements ResourceEngine component with factory pattern
pkg/core/discovery/*.go Implements ResourceDiscovery component with informer support
pkg/core/bootstrap/bootstrap.go Adds EventBus initialization to the bootstrap process
Various resource types Adds DeepCopyObject methods and factory functions to all resource types
Comments suppressed due to low confidence (2)

pkg/core/discovery/component.go:1

  • Missing init() function registration for the discoveryComponent. The component is not being registered with the runtime, so it won't be available for initialization.
/*

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@robocanic robocanic requested a review from Copilot August 24, 2025 09:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@robocanic robocanic requested a review from Copilot August 24, 2025 10:54

This comment was marked as outdated.

@robocanic robocanic requested a review from AlexStocks August 24, 2025 12:39
@robocanic
Copy link
Contributor Author

robocanic commented Aug 24, 2025

@锦涛 @凤瑞

@sonarqubecloud
Copy link

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the Informer Framework from client-go to support ResourceEngine and ResourceDiscovery components for list-watch functionality. The changes establish a comprehensive event-driven architecture for resource management in the Dubbo admin system.

  • Refactors resource registration to use factory functions and schema registry
  • Introduces EventBus component for event distribution with ResourceChangedEvent
  • Implements Informer pattern for watching resource changes with controller framework

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tools/resourcegen/main.go Updates resource code generation template to support DeepCopyObject and new factory patterns
pkg/core/resource/model/resource.go Adds k8sruntime.Object interface requirement to Resource
pkg/core/resource/model/registry.go Refactors from resource kind registry to schema registry with factory functions
pkg/core/resource/apis//v1alpha1/_types.go Implements DeepCopyObject methods and new resource factory functions
pkg/core/events/eventbus.go Replaces subscriber-based eventbus with resource-kind subscription manager
pkg/core/events/component.go New EventBus component implementation with synchronous event processing
pkg/core/controller/informer.go Implements informer pattern for watching and caching resource changes
pkg/core/engine/component.go ResourceEngine component that manages informers for infrastructure resources
pkg/core/discovery/component.go ResourceDiscovery component for service discovery with informers
pkg/core/bootstrap/bootstrap.go Adds EventBus initialization to bootstrap sequence
pkg/config/app/admin.go Updates discovery config to support multiple discovery configurations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link

@1kasa 1kasa left a comment

Choose a reason for hiding this comment

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

LGTM,The entire PR has an elegant code style, but I have a question: Why are the load order of the discovery/Component, engine/Component, and EventBus/Component components all set to math.MaxInt? Doesn't this make their startup order uncertain? Is there another way to ensure ordering? Or is ordering not really that important?
I'm very curious about this and would like to hear your thoughts.

@robocanic
Copy link
Contributor Author

LGTM,The entire PR has an elegant code style, but I have a question: Why are the load order of the discovery/Component, engine/Component, and EventBus/Component components all set to math.MaxInt? Doesn't this make their startup order uncertain? Is there another way to ensure ordering? Or is ordering not really that important? I'm very curious about this and would like to hear your thoughts.

At first, the start order of each component is hardcoded in bootstrap, so the start order is certain and ordered. The originial purpose of Order() defined in Component is to provide the start order: the larger the number, the earlier it starts. But if we defined a new component called A and
A is depend on Store, we need to look up the order of Store. if A is depend on many other components, we need to look up to the order of every components. So the design of OOrder() is not convenient enough. I have been looking for another way to resolve this issue.

@1kasa
Copy link

1kasa commented Aug 31, 2025

LGTM,The entire PR has an elegant code style, but I have a question: Why are the load order of the discovery/Component, engine/Component, and EventBus/Component components all set to math.MaxInt? Doesn't this make their startup order uncertain? Is there another way to ensure ordering? Or is ordering not really that important? I'm very curious about this and would like to hear your thoughts.LGTM,整个 PR 的代码风格都很优雅,但我有个疑问:为什么发现/Component、引擎/Component 和 EventBus/Component 组件的加载顺序都被设置为 math.MaxInt?这样难道不会让它们的启动顺序变得不确定吗?有没有其他方法来确保顺序?或者顺序真的没那么重要?我对此非常好奇,想听听你的想法。

At first, the start order of each component is hardcoded in bootstrap, so the start order is certain and ordered. The originial purpose of Order() defined in Component is to provide the start order: the larger the number, the earlier it starts. But if we defined a new component called A and最初,每个组件的启动顺序在 bootstrap 中是硬编码的,所以启动顺序是确定的且有序的。 Order()Component 中定义的初衷是为了提供启动顺序:数字越大,启动越早。但如果我们定义了一个新的组件叫 A ,并且 A is depend on Store, we need to look up the order of Store. if A is depend on many other components, we need to look up to the order of every components. So the design of OOrder() is not convenient enough. I have been looking for another way to resolve this issue. A 依赖于 Store ,我们需要查找 Store 的顺序。如果 A 依赖于许多其他组件,我们需要查找每个组件的顺序。所以 OOrder() 的设计不够方便。我一直在寻找另一种解决这个问题的方法。

Understood, thank you for your answer

@AlexStocks AlexStocks merged commit 166a692 into apache:develop Sep 3, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants