trpc-group / trpc-go

A pluggable, high-performance RPC framework written in golang
Other
742 stars 85 forks source link

plugin: Simplify global config by generic #156

Closed Andrew-M-C closed 5 months ago

Andrew-M-C commented 5 months ago

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.

tocrafty commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

after change:

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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