Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement max duration of HTTP ConnectionPools #5801

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Implement max duration of HTTP ConnectionPools #5801

merged 1 commit into from
Jan 12, 2021

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Dec 11, 2020

Closes #5799

@lorban lorban linked an issue Dec 11, 2020 that may be closed by this pull request
Allow specifying the duration an object can stay in a pool #5799
Closed
joakime
joakime reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads a lot easier.
Not many changes to multiplex needed?

Some unit tests would be next step (if @sbordet likes this approach)

jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved
@lorban lorban mentioned this pull request Dec 14, 2020
Closed
@lorban
Copy link
Contributor Author

lorban commented Dec 14, 2020

@joakime This PR can be considered a draft of what needs to be done, but it's not yet functional.

There's a new concept that the connection pool now needs to deal with: the fact that the pool can now contain a connection that is unusable because it expired. I'm busy writing some tests that expose this behavior, and they're miserably failing ATM.

@gregw
Copy link
Contributor

gregw commented Dec 14, 2020

@lorban I'd really like the Pool to not need to know about expiry and it should all be done in the ConnectionPool.
The ConnectionPool can check expiry whenever it acquires an entry and can also proactively sweep with acquire-check-release or check-acquire-recheck-release style

@joakime joakime marked this pull request as draft December 14, 2020 13:41
@lorban lorban force-pushed the jetty-9.4.x-5799-pool-max-duration2 branch from 44bc3ef to 302aad5 Compare December 14, 2020 14:37
@lorban
Copy link
Contributor Author

lorban commented Dec 14, 2020

@gregw as discussed, this is what's going to happen. I've just closed the pool PR to reflect this.

@lorban lorban force-pushed the jetty-9.4.x-5799-pool-max-duration2 branch from 302aad5 to 24d97b8 Compare December 14, 2020 14:49
@lorban lorban marked this pull request as ready for review December 14, 2020 14:50
@lorban
Copy link
Contributor Author

lorban commented Dec 14, 2020

Side note: while working on this, I noticed thatthe low-level pool was missing one bug fix for did not make it to the 9.4.x branch, and some closing behavior was not well tested. Maybe that should be moved to a different PR?

@lorban lorban requested review from joakime and gregw December 14, 2020 16:07
joakime
joakime approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me

@lorban lorban force-pushed the jetty-9.4.x-5799-pool-max-duration2 branch from 24d97b8 to 5b8246f Compare December 17, 2020 17:25
@lorban lorban requested a review from sbordet January 4, 2021 09:05
sbordet
sbordet requested changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorban as discussed, ConnectionHolder leaks as a type parameter to subclass constructors, which is not ideal.

Let's give a go at making Pool.Entry implement Attachable and store the duration inside that attachment, so the implementation is completely hidden in AbstractConnectionPool.

If maxDuration is disabled, we don't even attach anything, and we save an allocation.

@lorban
Copy link
Contributor Author

lorban commented Jan 6, 2021

@sbordet Since Connection already is attachable, instead of attaching Entry instances, we can attach an EntryHolder instance instead and leave Pool.Entry untouched.

I went that route as I find it cleaner, even if that means an unnecessary extra allocation when expiration isn't configured.

@lorban lorban requested a review from sbordet January 6, 2021 13:14
@lorban lorban force-pushed the jetty-9.4.x-5799-pool-max-duration2 branch from ab57c63 to e6ca0d9 Compare January 7, 2021 16:29
sbordet
sbordet requested changes Jan 8, 2021
View reviewed changes
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Show resolved Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
@lorban lorban requested a review from sbordet January 11, 2021 11:02
sbordet
sbordet requested changes Jan 12, 2021
View reviewed changes
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Show resolved Hide resolved
jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Outdated Show resolved Hide resolved
...ansport/src/test/java/org/eclipse/jetty/http2/client/http/MultiplexedConnectionPoolTest.java Show resolved Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java Outdated
@@ -105,6 +109,27 @@ protected void doStop() throws Exception
return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0]));
}

/**
* Get the max usage duration in milliseconds of the pool's connections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap with <p>.

@lorban lorban requested a review from sbordet January 12, 2021 11:34
@lorban lorban force-pushed the jetty-9.4.x-5799-pool-max-duration2 branch from d9a09f5 to 0dec883 Compare January 12, 2021 14:42
This was referenced Mar 10, 2021
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@sbordet sbordet sbordet approved these changes

@gregw gregw Awaiting requested review from gregw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow specifying the duration an object can stay in a pool
4 participants
@lorban @gregw @joakime @sbordet