search

cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.28k stars 1.1k forks source link

Expose currently pub(crate) configuration options, provide new Server constructor #165

Closed jamesmunns closed 2 months ago

jamesmunns commented 3 months ago

Partially addresses #159.

This makes the pub(crate) fields fully pub, and adds a new "manual" constructor.

This is a minor change, however it might be necessary in the future to decouple pingora-core from "configuration frontends", as discussed in the issue linked above.

The intent is to use this new interface with river, which will bring it's own configuration frontend.

drcaramelsyrup commented 3 months ago

My GitHub-fu is poor, so here's alternatively a branch to view the supplemental commits I'm adding per the comments, which will also get merged alongside this when we next sync commits.

jamesmunns commented 3 months ago

Thanks @drcaramelsyrup, it'd be good to have feedback on https://github.com/cloudflare/pingora/issues/159, specifically "carving out config", and separating "the config the Server needs" from "a YAML/serde format".

I think this would make the private/public fields more clear.

drcaramelsyrup commented 3 months ago

Yes, I think the issue behind the private/public fields in this case is that some of those fields are "conveniently" in ServerConf today which also serves as the config file format, whereas they might really belong in some ConnectorConf or ProxyConf as a barebones Server might not need, for example, the upstream connect config. Perhaps all the more reason to separate the config file concerns from the conf structs themselves.

andrewhavck commented 2 months ago

This has been merged, thanks!