Br/taco 385 - adding builders for ProxyRouteConfig and ClientConfig to make these at runtime #279
Conversation
…e easily create these objects at runtime while having the ability to use default config values
Rivukis
changed the title
| private InetSocketAddress remote; | ||
| private IdleTimeoutConfig idleTimeoutConfig; | ||
|
|
||
| private Builder(ClientConfig fallbackObject) { |
There was a problem hiding this comment.
This is a ClientConfig instead of a Config so that the user has the capabilities to create a default object once and use it over and over instead of newing up one everytime with the same Config
There was a problem hiding this comment.
Same reasoning in ProxyRouteConfig as well
| ClientConfig fallbackObject = new ClientConfig(config.getConfig("xio.clientTemplate")); | ||
| Map expectedBootstrapOptions = Maps.newHashMap(ChannelOption.AUTO_READ, new Object()); | ||
| String expectedName = "name"; | ||
| TlsConfig expectedTls = new TlsConfig(ConfigFactory.load("tls-reference")); |
There was a problem hiding this comment.
I'm trying to avoid having to make a builder for TlsConfig for now in hopes that we won't need one. If we need to be able to create different ones at runtime, we can add it then
There was a problem hiding this comment.
You shouldn’t need a builder for TlsConfig. We’ll always want something typesafe config based so we can pull in defaults from reference.conf
There was a problem hiding this comment.
That's what I was hoping for 👍
| * <p>If a value is not set, it defaults to using the default object's value. | ||
| */ | ||
| public static class Builder { | ||
| private ClientConfig fallbackObject; |
There was a problem hiding this comment.
The name is fallbackObject the javadoc says default object. I would pick one name and stick with it.
| this.fallbackObject = fallbackObject; | ||
| } | ||
|
|
||
| public Builder setBootstrapOptions(Map<ChannelOption<Object>, Object> bootstrapOptions) { |
There was a problem hiding this comment.
If I was writing this from scratch i’d use a google auto builder. Don’t change anything here. Just look into it for next time so you don’t have to create a bunch of boilerplate. Then again. I’m hand crafting code with sed and you’re using intellij so i’m sure this was painless.
There was a problem hiding this comment.
Oh no, it was painful. I just didn't know about auto builder. I'll look into it
There was a problem hiding this comment.
looked into it. got close to making it work, but it's not compiling and the output is not showing me the error. I'm out of ideas so moving forward with my hand rolled version
| * <p>If a value is not set, it defaults to using the default object's value. | ||
| */ | ||
| public static class Builder { | ||
| private ProxyRouteConfig fallbackObject; |
There was a problem hiding this comment.
Same note as before either default or fallback as the name.
| ClientConfig fallbackObject = new ClientConfig(config.getConfig("xio.clientTemplate")); | ||
| Map expectedBootstrapOptions = Maps.newHashMap(ChannelOption.AUTO_READ, new Object()); | ||
| String expectedName = "name"; | ||
| TlsConfig expectedTls = new TlsConfig(ConfigFactory.load("tls-reference")); |
There was a problem hiding this comment.
You shouldn’t need a builder for TlsConfig. We’ll always want something typesafe config based so we can pull in defaults from reference.conf
| @@ -209,12 +209,6 @@ xio { | |||
| name = "test client" | |||
| } | |||
|
|
|||
| proxyRouteTemplate = ${xio.routeTemplate} { | |||
There was a problem hiding this comment.
Nice. We missed this before.
No description provided.