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

Rewrite host connection pool as single GraphStage #1312

Closed
jrudolph opened this Issue Jul 27, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@jrudolph
Member

jrudolph commented Jul 27, 2017

As far as I can see that should remove lots of the stream related bugs. The custom GraphStage should contain all logic from PoolInterfaceActor, PoolConductor, PoolSlot. The external interface should probably be a FlowShape[HttpRequest, HttpResponse]. The PoolMasterActor can then use that instead of sending messages to the current PoolInterfaceActor.

I guess in the best case, we would make this new implementation available under a feature flag depending on our confidence on getting it right.

@tomrf1

This comment has been minimized.

Show comment
Hide comment
@tomrf1

tomrf1 Oct 1, 2017

Contributor

This sounds like an interesting one.
The PoolInterfaceActor currently has a request buffer with size max-open-requests. Is the intention to continue using the buffer, inside the new GraphStage? (Perhaps as a rate decoupled graph stage).
I'm not sure what the reason for the buffering here is. Why not just backpressure all the way and let the user decide whether or not to buffer?

Contributor

tomrf1 commented Oct 1, 2017

This sounds like an interesting one.
The PoolInterfaceActor currently has a request buffer with size max-open-requests. Is the intention to continue using the buffer, inside the new GraphStage? (Perhaps as a rate decoupled graph stage).
I'm not sure what the reason for the buffering here is. Why not just backpressure all the way and let the user decide whether or not to buffer?

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 4, 2017

Member

@tomrf1, good point. The reason for the buffer is that requests can always be pushed towards a pool by running them via Http.singleRequest. I think previously there was also another concern that new stream materializations created by Http.cachedHostConnectionPool or Http.superPool also need some buffer space because a pool cannot know from which of those materializations to request from, so it requests from all of them.

I think we might want to separate the effort into a part of just rewriting PoolConductor, PoolFlow, and PoolSlot leaving PoolInterfaceActor as it is.

Rewriting PoolInterfaceActor into something else could be another step in the effort.

Member

jrudolph commented Oct 4, 2017

@tomrf1, good point. The reason for the buffer is that requests can always be pushed towards a pool by running them via Http.singleRequest. I think previously there was also another concern that new stream materializations created by Http.cachedHostConnectionPool or Http.superPool also need some buffer space because a pool cannot know from which of those materializations to request from, so it requests from all of them.

I think we might want to separate the effort into a part of just rewriting PoolConductor, PoolFlow, and PoolSlot leaving PoolInterfaceActor as it is.

Rewriting PoolInterfaceActor into something else could be another step in the effort.

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 6, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 9, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 9, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

+htc akka#1312 new host connection pool implementation
A new implementation of the HostConnectionPool. The basic idea is to replace
the former complicated streaming pipeline of `PoolFlow`, `PoolConductor`,
and `PoolSlot` into a single stage that handles all aspects to get rid
of all the small race condition issues that exist in the current ("legacy")
pool implementation.

The new pool implementation is split into two basic classes

 * akka.http.impl.engine.client.pool.NewHostConnectionPool that provides all
   the infrastructure and event handling to drive the pool
 * akka.http.impl.engine.client.pool.SlotState that contains only the logic
   to handle state changes of a single pool slot

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

+htc akka#1312 new host connection pool implementation
A new implementation of the HostConnectionPool. The basic idea is to replace
the former complicated streaming pipeline of `PoolFlow`, `PoolConductor`,
and `PoolSlot` into a single stage that handles all aspects to get rid
of all the small race condition issues that exist in the current ("legacy")
pool implementation.

The new pool implementation is split into two basic classes

 * akka.http.impl.engine.client.pool.NewHostConnectionPool that provides all
   the infrastructure and event handling to drive the pool
 * akka.http.impl.engine.client.pool.SlotState that contains only the logic
   to handle state changes of a single pool slot

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

+htc akka#1312 new host connection pool implementation
A new implementation of the HostConnectionPool. The basic idea is to replace
the former complicated streaming pipeline of `PoolFlow`, `PoolConductor`,
and `PoolSlot` into a single stage that handles all aspects to get rid
of all the small race condition issues that exist in the current ("legacy")
pool implementation.

The new pool implementation is split into two basic classes

 * akka.http.impl.engine.client.pool.NewHostConnectionPool that provides all
   the infrastructure and event handling to drive the pool
 * akka.http.impl.engine.client.pool.SlotState that contains only the logic
   to handle state changes of a single pool slot

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

+htc akka#1312 new host connection pool implementation
A new implementation of the HostConnectionPool. The basic idea is to replace
the former complicated streaming pipeline of `PoolFlow`, `PoolConductor`,
and `PoolSlot` into a single stage that handles all aspects to get rid
of all the small race condition issues that exist in the current ("legacy")
pool implementation.

The new pool implementation is split into two basic classes

 * akka.http.impl.engine.client.pool.NewHostConnectionPool that provides all
   the infrastructure and event handling to drive the pool
 * akka.http.impl.engine.client.pool.SlotState that contains only the logic
   to handle state changes of a single pool slot

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 30, 2017

+htc akka#1312 new host connection pool implementation
A new implementation of the HostConnectionPool. The basic idea is to replace
the former complicated streaming pipeline of `PoolFlow`, `PoolConductor`,
and `PoolSlot` into a single stage that handles all aspects to get rid
of all the small race condition issues that exist in the current ("legacy")
pool implementation.

The new pool implementation is split into two basic classes

 * akka.http.impl.engine.client.pool.NewHostConnectionPool that provides all
   the infrastructure and event handling to drive the pool
 * akka.http.impl.engine.client.pool.SlotState that contains only the logic
   to handle state changes of a single pool slot

jrudolph added a commit that referenced this issue Nov 30, 2017

Merge pull request #1533 from jrudolph/jr/w/1312-rewrite-poolslot
#1312 Rewrite of PoolFlow / PoolSlot / PoolConductor as single GraphStage

@jrudolph jrudolph added this to the 10.0.11 milestone Nov 30, 2017

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 30, 2017

Member

The new pool implementation will be available in 10.0.11 under a flag. I'm closing this one in favor of follow up tickets to flesh out details of behavior in the new implementation when it got some more testing.

Member

jrudolph commented Nov 30, 2017

The new pool implementation will be available in 10.0.11 under a flag. I'm closing this one in favor of follow up tickets to flesh out details of behavior in the new implementation when it got some more testing.

@jrudolph jrudolph closed this Nov 30, 2017

@benjamingeer

This comment has been minimized.

Show comment
Hide comment
@benjamingeer

benjamingeer Jun 6, 2018

Is the new pool implementation still available only under a flag in 10.1.1, and if so, what's the flag? And does it resolve #1459?

benjamingeer commented Jun 6, 2018

Is the new pool implementation still available only under a flag in 10.1.1, and if so, what's the flag? And does it resolve #1459?

@raboof

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Jun 6, 2018

Member

The new host connection pool is the default since 10.1.0. I'm not familiar enough with #1459 to say whether it is expected to resolved (though if so of course that ticket should be closed).

Member

raboof commented Jun 6, 2018

The new host connection pool is the default since 10.1.0. I'm not familiar enough with #1459 to say whether it is expected to resolved (though if so of course that ticket should be closed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment