A few pool improvements #1660

jrudolph · 2017-12-19T16:22:58Z

no unbounded response buffers, in a code comment I previously speculated whether buffering would be ok or not. I figured, probably unbounded buffering is not so cool so I removed it. Now the outgoing result is buffered in the slot until it is fetched
some additional failure handlers

akka-ci · 2017-12-19T16:30:10Z

Test PASSed.

akka-ci · 2017-12-21T13:39:58Z

Test PASSed.

ktoso · 2017-12-21T13:44:30Z

akka-http-core/src/main/scala/akka/http/impl/engine/client/pool/SlotState.scala

    }

    override def onConnectionFailed(ctx: SlotContext, cause: Throwable): SlotState = {
      ctx.debug("Connection failed.")
      // FIXME: register failed connection attempt, schedule request for rerun, backoff new connection attempts
      ctx.closeConnection()
-      ctx.dispatchFailure(ongoingRequest, cause)
-      Unconnected
+      // FIXME: register failed connection attempt, backoff new connection attempts


It's basically #1391 but I'll add it to the comment.

ktoso · 2017-12-21T13:46:18Z

akka-http-core/src/test/scala/akka/http/impl/engine/client/HostConnectionPoolSpec.scala

+
+        eventually {
+          a[Throwable] should be thrownBy oneCycle()
+        }


what's the error message here?

A timeout that a push / expect didn't go through because of backpressure. Hard to test otherwise.

Testing method is fine, just wondering if the message is understandable :)

Added a comment.

ktoso

Looks fine, one question

akka-ci · 2017-12-21T13:54:57Z

Test FAILed.

ktoso · 2017-12-21T13:59:40Z

akka-http-core/src/test/scala/akka/http/impl/engine/client/HostConnectionPoolSpec.scala

+
+        eventually {
+          // should fail eventually because backpressure kicks in and one of the expects / pushes above will timeout
+          a[Throwable] should be thrownBy oneCycle()


akka-ci · 2017-12-21T13:59:59Z

Test PASSed.

ktoso · 2017-12-22T10:30:59Z

akka-http-core/src/main/scala/akka/http/impl/engine/client/pool/SlotState.scala

@@ -34,6 +36,8 @@ private[pool] abstract class SlotContext {

  def debug(msg: String): Unit
  def debug(msg: String, arg1: AnyRef): Unit
+  def debug(msg: String, arg1: AnyRef, arg2: AnyRef): Unit
+  def debug(msg: String, arg1: AnyRef, arg2: AnyRef, arg3: AnyRef): Unit


ktoso · 2017-12-22T10:35:04Z

akka-http-core/src/main/scala/akka/http/impl/engine/client/pool/SlotState.scala

  }
  final case class WaitingForResponse(ongoingRequest: RequestContext) extends ConnectedState with BusyState {
    override def onResponseReceived(ctx: SlotContext, response: HttpResponse): SlotState =
      WaitingForResponseDispatch(ongoingRequest, Success(response))

-    override def onConnectionFailed(ctx: SlotContext, cause: Throwable): SlotState =
-      WaitingForResponseDispatch(ongoingRequest, Failure(cause))
+    // connection failures are handled by BusyState implementations


thanks for leaving those comments 👍

ktoso · 2017-12-22T10:40:09Z

Looked at all commits one by one and changes look good. LGTM

…ation

…istent

…per request

akka-ci · 2017-12-22T11:38:23Z

Test FAILed.

jrudolph · 2017-12-22T12:13:02Z

PLS BUILD

akka-ci · 2017-12-22T12:20:05Z

Test FAILed.

jrudolph · 2017-12-22T12:28:05Z

PLS BUILD

akka-ci · 2017-12-22T12:35:11Z

Test PASSed.

jrudolph · 2017-12-22T12:38:46Z

Finally ;)

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Dec 19, 2017

jrudolph force-pushed the jr/w/review-new-pool branch from 9aea8eb to 7c5b0b5 Compare December 21, 2017 13:33

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Dec 21, 2017

ktoso reviewed Dec 21, 2017

View reviewed changes

ktoso approved these changes Dec 21, 2017

View reviewed changes

jrudolph force-pushed the jr/w/review-new-pool branch 2 times, most recently from d4202d2 to 478d8c0 Compare December 21, 2017 13:50

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Dec 21, 2017

akka-ci added validating PR that is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Dec 21, 2017

jrudolph force-pushed the jr/w/review-new-pool branch from 478d8c0 to 8422111 Compare December 21, 2017 13:55

ktoso reviewed Dec 21, 2017

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Dec 21, 2017

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins labels Dec 21, 2017

ktoso reviewed Dec 22, 2017

View reviewed changes

jrudolph added 5 commits December 22, 2017 12:23

=htc remove unbounded response buffering in new client pool implement…

b707aff

…ation

=htc harden state machine by adding missing handlers

abed861

=htc fix a few warnings in new host pool implementation

7818468

=htc try to make error handling in new host connection pool more cons…

b26505c

…istent

=htc ClientServerSpec: simplify debugging by sending different bytes …

ae9df09

…per request

jrudolph force-pushed the jr/w/review-new-pool branch from ec264c8 to ae9df09 Compare December 22, 2017 11:23

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Dec 22, 2017

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Dec 22, 2017

jrudolph merged commit 3d4ecc4 into akka:master Dec 22, 2017

jrudolph deleted the jr/w/review-new-pool branch December 22, 2017 12:39

jrudolph added this to the 10.1.0-RC1 milestone Dec 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few pool improvements #1660

A few pool improvements #1660

jrudolph commented Dec 19, 2017

akka-ci commented Dec 19, 2017

akka-ci commented Dec 21, 2017

ktoso Dec 21, 2017

jrudolph Dec 21, 2017

ktoso Dec 21, 2017

jrudolph Dec 21, 2017

ktoso Dec 21, 2017

jrudolph Dec 21, 2017

ktoso left a comment

akka-ci commented Dec 21, 2017

ktoso Dec 21, 2017

akka-ci commented Dec 21, 2017

ktoso Dec 22, 2017

ktoso Dec 22, 2017

ktoso commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017

A few pool improvements #1660

A few pool improvements #1660

Conversation

jrudolph commented Dec 19, 2017

akka-ci commented Dec 19, 2017

akka-ci commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

akka-ci commented Dec 21, 2017

Choose a reason for hiding this comment

akka-ci commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017

akka-ci commented Dec 22, 2017

jrudolph commented Dec 22, 2017