-
Notifications
You must be signed in to change notification settings - Fork 79
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
#235 - Removed unused method #241
#235 - Removed unused method #241
Conversation
/** | ||
* Create a new builder. | ||
* | ||
* @return a new builder | ||
*/ | ||
public static Builder newHttpClientBuilder(BackendService backendService) { | ||
return new Builder(backendService); | ||
public static Builder newHttpClientBuilder(Id applicationId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should call the parameter backendServiceId
for consistency.
|
||
public Builder(BackendService backendService) { | ||
this.backendService = checkNotNull(backendService); | ||
public Builder(Id applicationId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should call the parameter and field backendServiceId
for consistency.
4a64f5a
to
9f58868
Compare
public Builder(BackendService backendService) { | ||
this.backendService = checkNotNull(backendService); | ||
public Builder(Id backendServiceId) { | ||
this.backendServiceId = checkNotNull(backendServiceId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Java 8's requireNonNull
instead of Guava's checkNotNull
. Same comment for stickySessionConfig(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See ticket #243
components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java
Show resolved
Hide resolved
62e5322
to
29a2156
Compare
val client = newHttpClientBuilder(backendService.id) | ||
.loadBalancer(stickySessionStrategy(activeOrigins(backendService))) | ||
.stickySessionConfig(stickySessionConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear how to do this properly/generally in our case, but we might want to test that the required 'Builder' methods (in this case, stickySessionConfig() ) have been invoked in our UnitTests instead of only during the EndToEndTests . Ideally, we would guarantee this at compile time or at the very least at runtime.
This is a clear short-coming of our usage of the builder pattern, we cannot guarantee that the object has been constructed with all the required parameters...
Removed unused
isHttps()
and removedBackendService
constructor argument fromStyxHttpClient.Builder