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

plugin: Simplify global config by generic #156

Closed
Andrew-M-C opened this issue Jan 8, 2024 · 6 comments
Closed

plugin: Simplify global config by generic #156

Andrew-M-C opened this issue Jan 8, 2024 · 6 comments
Assignees
Labels
NeedsInvestigation The issue is not fully understood and requires analysis to understand the root cause

Comments

@Andrew-M-C
Copy link
Contributor

Is your feature request related to a problem?

When I want to get configurations from default global trpc_go.yaml file, I had to create a type, implementing plugin.Factory.

For simple confiration, these codes is quite annoying an unnecessary.

What is the solution you'd like?

A simple function call, parsing a callback or simply a local address binding for yaml unmarshaling.

What alternatives have you considered?

Please refer to my implemtation: https://github.com/Andrew-M-C/trpc-go-utils/blob/master/config/config.go

Could you provide additional context?

Please review my implementation. I can also raise a PR.

@Andrew-M-C Andrew-M-C changed the title plugin: Simplify global config receiver by generic plugin: Simplify global config by generic Jan 8, 2024
@liuzengh liuzengh self-assigned this Jan 9, 2024
@liuzengh liuzengh added the NeedsInvestigation The issue is not fully understood and requires analysis to understand the root cause label Jan 9, 2024
@tocrafty
Copy link
Contributor

tocrafty commented Jan 9, 2024

You proposed a functional registry, which IMHO, doesn't differ significantly from Object-Oriented (OO) design.
With OO desgin, we can cohere not only Setup, but also dependencies, FinishNotifier and Closer in a single struct. To simulate them in functaional design, your implementaiton need to be enriched with more functional options.
Generic and interface are just two forms of abstraction. Generic works at compiler level, while interface works at runtime. Your generic version fall backs to interface in the internal implementation.
You need to define your receiver function some where, just like defining a plugin method. Though a lambda may ease the definition of receiver, the struct T can not be ommited.
I believe it offers only marginal improvement.
Let's see how others think of this. @sandyskies

@WineChord
Copy link
Contributor

WineChord commented Jan 9, 2024

In my humble opinion, unless the new style offers significant improvements over the existing approach, it should not be added to the main repository.

This is because we cannot deprecate the existing exported interfaces, which are heavily used by the ecosystem. While the new style may simplify certain use cases, additional documentation would be necessary to educate users. Consequently, we would have several different ways to accomplish the same task, which could create additional overhead for new users (consider the numerous ways to do the same thing in C++).

In summary, unless the new style is far superior to the current practice, I would recommend incorporating such features into users' own codebases (or as part of the trpc-ecosystem?) rather than integrating them into the main repository.

@liuzengh
Copy link
Contributor

liuzengh commented Jan 9, 2024

I agree tocrafty‘s opinion.

You proposed a functional registry, which IMHO, doesn't differ significantly from Object-Oriented (OO) design. With OO desgin, we can cohere not only Setup, but also dependencies, FinishNotifier and Closer in a single struct. To simulate them in functaional design, your implementaiton need to be enriched with more functional options. Generic and interface are just two forms of abstraction. Generic works at compiler level, while interface works at runtime. Your generic version fall backs to interface in the internal implementation. You need to define your receiver function some where, just like defining a plugin method. Though a lambda may ease the definition of receiver, the struct T can not be ommited. I believe it offers only marginal improvement. Let's see how others think of this. @sandyskies

before change:

  • disadvantage: requires manual implementation of the Setup function for each plugin, which can lead to code redundancy if there are many plugins.

after change:

  • advantage: avoids plugin developers writing nearly identical Setup logic by embedding Setup logic within the framework.

Why does the code before the change have the disadvantage? Could the original approach have been avoided from the beginning?

If inheritance is possible, the subclass can directly inherit the parent class's Setup method without explicitly overriding it. When the subclass does not override the parent class's method, the subclass object can directly call the parent class's method. In addition, in C++, the subclass can use the scope resolution operator "::" to invoke the parent class's method. By using the syntax "ParentClassName::MethodName" within the subclass, the parent class's method can be accessed directly.

In Go language, there is no concept of classes. Instead, it uses structs and interfaces to achieve object composition and behavior definition. In Go, structs can be embedded within other structs to achieve a similar effect to inheritance.

// "trpc.group/trpc-go/trpc-go/config_util"
type PluginFactory struct {
    typ, name string
    plugin    interface{}
}

// Type 实现 plugin.Factory
func (p *PluginFactory) Type() string {
    return p.typ
}

// Setup 实现 plugin.Factory
func (p *PluginFactory) Setup(name string, decoder plugin.Decoder) error {
    if err := decoder.Decode(p.plugin); err != nil {
        return fmt.Errorf("decode config %s.%s error: '%w'", p.typ, p.name, err)
    }
    return nil
}
// CustomPlugin struct implements plugin.Factory interface.
type CustomPlugin struct {
    cutil.PluginFactory
}

func (p *CustomPlugin) Setup(name string, dec plugin.Decoder) error {
    if err := p.PluginFactory.Setup(name, dec); err != nil {
        return err
    }
    // do something
    return nil
}

@Andrew-M-C
Copy link
Contributor Author

Actually my proposal did not focus on generics, any may also work. Genericing is just a simple way to tell diffent auto-generated-plugin factories apart.

Why I suggest this 2 simple functions is because according our praciting experience, in most cases, what we need is just reading configs from yaml file. Everytime we want to achieve that, we had to write a bunch of codes to implement it. With code I provided, less codes will be needed.

It was already an utility in our team code repo and team members suggest me raising this PR to main. If you think this is quite against the design pattern of plugins, please feel free to close this issue. :-)

@tocrafty
Copy link
Contributor

At the sight of software engineering, reusing plugin to do yaml parsing is not a good idea, and so does storing your bussiness config in trpc_go.yaml.
To parse a sepecific sub-fields from yaml, you can use reflect. This more clean than plugin.

func Unmarshal(in []byte, fields []string, out interface{}) error {
	if len(fields) == 0 {
		return yaml.Unmarshal(in, out)
	}
	var node yaml.Node
	if err := yaml.Unmarshal(in, &node); err != nil {
		return err
	}
	return unmarshal(&node, fields, out)
}

func unmarshal(node *yaml.Node, fields []string, out interface{}) error {
	if len(fields) == 0 {
		return node.Decode(out)
	}
	var t trait
	f := reflect.TypeOf(t).Field(0)
	f.Tag = reflect.StructTag(fmt.Sprintf(`yaml:"%s"`, fields[0]))
	if err := node.Decode(reflect.ValueOf(&t).Convert(
		reflect.PointerTo(reflect.StructOf([]reflect.StructField{f})),
	).Interface()); err != nil {
		return err
	}

	return unmarshal(&t.Value, fields[1:], out)
}

type trait struct {
	Value yaml.Node `yaml:"-"`
}

@Andrew-M-C
Copy link
Contributor Author

That is true. Plugin is used to configure PLUGINs, not business configuration. OK, issue closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation The issue is not fully understood and requires analysis to understand the root cause
Projects
None yet
Development

No branches or pull requests

4 participants