Refactor StyxHostHttpClient #338
Conversation
- Origin ID header attachment - Application name in the exceptions
components/client/src/main/java/com/hotels/styx/client/StyxBackendServiceClient.java
Outdated
Show resolved
Hide resolved
components/client/src/main/java/com/hotels/styx/client/StyxBackendServiceClient.java
Outdated
Show resolved
Hide resolved
components/client/src/main/java/com/hotels/styx/client/StyxBackendServiceClient.java
Outdated
Show resolved
Hide resolved
components/client/src/main/java/com/hotels/styx/client/Transport.java
Outdated
Show resolved
Hide resolved
Remove unnecessary NoAvailableHostsException mapping from StyxBackendServiceClient. Address code review comments.
components/client/src/main/java/com/hotels/styx/client/Transport.java
Outdated
Show resolved
Hide resolved
components/client/src/main/java/com/hotels/styx/client/Transport.java
Outdated
Show resolved
Hide resolved
| if (connection != null && connectionPool.isPresent()) { | ||
| connectionPool.get().returnConnection(connection); | ||
| } | ||
| private synchronized void returnIfConnected(ConnectionPool connectionPool, Connection connection) { |
There was a problem hiding this comment.
Don't we ever need to synchronize on something different than the current object? In that case, do we actually need to set 'closeIfConnected' and/or 'returnIfConnected' as synchronized methods or should we try to ensure they are idempotent?
There was a problem hiding this comment.
Hey David. There is no longer need to synchronise on anything. I realised myself the synchronized modifiers were left in the methods. They should have been removed already. Perhaps you have commented an outdated version?
There was a problem hiding this comment.
Thinking about it, perhaps I should remove these methods altogether.
There was a problem hiding this comment.
oh, sure, I see your commit now right before my comment. You can remove them but I don't see too much of a difference between both options to be honest... I don't dislike the 'extra indirection' myself.
| .apply(); | ||
| }); | ||
| } | ||
|
|
||
| private synchronized void closeIfConnected(Optional<ConnectionPool> connectionPool, Connection connection) { | ||
| if (connection != null && connectionPool.isPresent()) { |
There was a problem hiding this comment.
Have we confirmed that the connections are never null? It would make sense, but thought it worth checking.
There was a problem hiding this comment.
A null value wouldn't be emitted under any circumstances (that would be a bug elsewhere in styx).
Note that the Reactive Streams specification specifically forbids null values as events (Rule 2.13 https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.2/README.md). Although we are still in RxJava 1.0, our intention is to fully migrate to reactive-streams compatible implementation (Flux), ensuring it cannot be null.
Move functionality up into
StyxBackendServiceClient:Rationale: to make it easier to expose
StyxHostHttpClientas a reusable component for 3rd party projects. Specifically, this improvement removes classes likeOriginfrom the API and removes Styx proxy specific functionality like attaching origin IDs to HTTP responses.