Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.2] The configuration default value flow problem #2592

Closed
FoghostCn opened this issue Feb 2, 2024 · 2 comments
Closed

[3.2] The configuration default value flow problem #2592

FoghostCn opened this issue Feb 2, 2024 · 2 comments
Assignees

Comments

@FoghostCn
Copy link
Contributor

FoghostCn commented Feb 2, 2024

Issue description

The configuration default value flow problem, instanceOptions -> ServerOptions/ClientOptions -> ServiceOptions is the default value flow direction, and should not flow in the reverse direction. Each level inherits the default value of the previous level and can be modified at the current level. It should not affect the previous level. level value, the current implementation will synchronize back to InstanceOptions when modifying some configurations at the server layer.

配置的默认值流向问题,instanceOptions -> ServerOptions/ClientOptions -> ServiceOptions 为默认值流向,而不应该反向流动,每一级继承上一级的默认值,可以在当前层级修改,不应该影响上一级的值,目前的实现 在 server 层修改一些配置会同步回 InstanceOptions

@setcy
Copy link
Contributor

setcy commented Feb 12, 2024

I'm interested in this issue, can it be assigned to me?

Here is my proposal:

Background

// NewClient is like client.NewClient, but inject configurations from RootConfig and
// ConsumerConfig
func (ins *Instance) NewClient(opts ...client.ClientOption) (*client.Client, error) {
	if ins == nil || ins.insOpts == nil {
		return nil, errors.New("Instance has not been initialized")
	}

	var cliOpts []client.ClientOption
	conCfg := ins.insOpts.Consumer
	appCfg := ins.insOpts.Application
	regsCfg := ins.insOpts.Registries
	sdCfg := ins.insOpts.Shutdown
	metricsCfg := ins.insOpts.Metrics
	otelCfg := ins.insOpts.Otel
        ...
}

Proposal

In the current implementation of the NewClient, NewServer method, since configurations are passed directly to methods via pointers. Since the pointer refers to the same object, any modifications to the configuration options are reflected back on the original Instance object.

To address the aforementioned issues, I consider to use deep copied configurations to create clients, and implement respective deep copy functions in each type of configuration item. The design code will look like this:

	conCfg := ins.insOpts.CloneConsumer()
	appCfg := ins.insOpts.CloneApplication()
	regsCfg := ins.insOpts.CloneRegistries()
	sdCfg := ins.insOpts.CloneShutdown()
	metricsCfg := ins.insOpts.CloneMetrics()
	otelCfg := ins.insOpts.CloneOtel()

setcy added a commit to setcy/dubbo-go that referenced this issue Feb 14, 2024
…Options/ClientOptions

Fixes: apache#2592

Signed-off-by: setcy <asetcy@gmail.com>
@setcy
Copy link
Contributor

setcy commented Feb 14, 2024

Regarding the above proposal, I simply wrote draft pulll request #2596. Please check if it is as expected. If so, please assign this issue to me and I will continue to complete the remaining work, such as Checks and Tests.

FoghostCn pushed a commit to FoghostCn/dubbo-go that referenced this issue Mar 27, 2024
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

No branches or pull requests

2 participants