-
Notifications
You must be signed in to change notification settings - Fork 596
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
Revamp SPI / API of ClientTransport infrastructure to fix #1012 and to simplify API #1195
Revamp SPI / API of ClientTransport infrastructure to fix #1012 and to simplify API #1195
Conversation
…tings to Java class
A transport is now a static constant function value that gets passed all parameters at connection time. That fixes akka#1012 where client connection settings were both kept redundantly inside the transport and inside the settings. It's now also possible to specify the localAddress inside the settings. Methods in the Http entrypoint that allow specifying the localAddress explicitly will override whatever was given in the settings. In the future we'd like to get rid of all those parameters to simplify entrypoint signatures in Http.
Test PASSed. |
5aa8d17
to
d87e2a8
Compare
Test FAILed. |
PLS BUILD |
Test PASSed. |
Yeah... though perhaps won't been much more of a problem than it is already in practice; For now we only have the Proxy coming up there AFAICS; Technically we could pull off some trick like transportSettings: Map[TransportSettingKey[V], V] or something if we care about avoiding more fields in there etc (would allow keeping external projects to contribute transports - though I'm not sure that will happen or should be a goal - what was your plan about that?) |
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.
LGTM, thanks for getting back to it swiftly; Core of the issue addressed nicely as well, which I back then referred to as the "passing around settings problem" and done in pretty clean way :-)
@@ -2,3 +2,35 @@ | |||
|
|||
# Added a new method to this sealed trait | |||
ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.http.javadsl.model.MediaType#WithOpenCharset.toContentTypeWithMissingCharset") | |||
|
|||
# Move settings implementations to Java superclass, ClientConnectionSettings are marked @DoNotInherit |
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.
thanks for the meaningful comments on those filters 👍
private case class TCPTransport(localAddress: Option[InetSocketAddress], settings: ClientConnectionSettings) extends ClientTransport { | ||
def connectTo(host: String, port: Int)(implicit system: ActorSystem): Flow[ByteString, ByteString, Future[OutgoingConnection]] = | ||
private case object TCPTransport extends ClientTransport { | ||
def connectTo(host: String, port: Int, settings: ClientConnectionSettings)(implicit system: ActorSystem): Flow[ByteString, ByteString, Future[OutgoingConnection]] = |
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.
Yeah, this solves the problem of it being passed in "too early and only once", good.
@@ -297,7 +297,7 @@ class HttpExt(private val config: Config)(implicit val system: ActorSystem) exte | |||
localAddress: Option[InetSocketAddress] = None, | |||
settings: ClientConnectionSettings = ClientConnectionSettings(system), | |||
log: LoggingAdapter = system.log): Flow[HttpRequest, HttpResponse, Future[OutgoingConnection]] = | |||
_outgoingConnection(host, port, settings, ConnectionContext.noEncryption(), ClientTransport.TCP(localAddress, settings), log) | |||
_outgoingConnection(host, port, settings.withLocalAddressOverride(localAddress), ConnectionContext.noEncryption(), ClientTransport.TCP, log) |
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.
good, makes sense; this is where the passing in of the settings previously did not do what was expected AFAIR
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.
No, the problem was that ConnectionPoolSettings.withConnectionSettings(...)
had no effect because the settings had already been passed to the transport.
*/ | ||
def withLocalAddressOverride(overrideLocalAddressOption: Option[InetSocketAddress]): ClientConnectionSettings = | ||
if (overrideLocalAddressOption.isDefined) withLocalAddress(overrideLocalAddressOption) | ||
else this | ||
} |
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.
good 👍
final override def getRequestHeaderSizeHint: Int = requestHeaderSizeHint | ||
final override def getWebsocketRandomFactory: Supplier[Random] = new Supplier[Random] { | ||
override def get(): Random = websocketRandomFactory() | ||
} |
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.
moving them up one level looks good 👍
val poolSettings = | ||
ConnectionPoolSettings(system) | ||
.withTransport(transport) | ||
.withConnectionSettings(ClientConnectionSettings(system).withIdleTimeout(23.hours)) |
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.
extract the 23.hours
as value? a bit easier to see where it's set / expected then?
transport.out.sendComplete() | ||
|
||
response.entity.dataBytes.utf8String.awaitResult(10.seconds) should ===("Hello World!") | ||
} |
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.
Great test setup!
d87e2a8
to
73f48bd
Compare
Test PASSed. |
This was inspired by part of @ktoso's fix in #1014 but goes a bit further to simplify the transport API.
A transport is now a static constant function value that gets passed
all parameters at connection time.
That fixes #1012 where client connection settings were both kept redundantly
inside the transport and inside the settings.
It's now also possible to specify the localAddress inside the settings. Methods
in the Http entrypoint that allow specifying the localAddress explicitly will
override whatever was given in the settings. In the future we'd like to
get rid of all those parameters to simplify entrypoint signatures in Http.
There is a basic disadvantage of passing
ClientConnectionSettings
to theconnectTo
method which is that whatever settings futureClientTransport
come up with, they have to be put into thoseClientConnectionSettings
, so this might become an even bigger grab bag than it is right now.Fixes #1012.