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

Allow configurable connection provider eviction predicate #2557

Closed
wants to merge 1 commit into from
Closed

Allow configurable connection provider eviction predicate #2557

wants to merge 1 commit into from

Conversation

samueldlightfoot
Copy link
Contributor

@samueldlightfoot samueldlightfoot commented Nov 1, 2022 •
edited

Add support for a custom eviction predicate in the ConnectionProvider.

@samueldlightfoot samueldlightfoot force-pushed the eviction-predicate branch 3 times, most recently from 68dfe0b to 3851363 Compare November 1, 2022 19:55
@samueldlightfoot samueldlightfoot force-pushed the eviction-predicate branch 5 times, most recently from d307ad6 to ab4d4ac Compare November 9, 2022 22:53
@violetagg
Copy link
Member

@samueldlightfoot I'll check this in the next days

@violetagg
Copy link
Member

@samueldlightfoot For HTTP/1.1 you ignore the default eviction predicate and max idle/life time, but for HTTP/2 you keep them. Why do you introduce this difference?

@samueldlightfoot
Copy link
Contributor Author

@samueldlightfoot For HTTP/1.1 you ignore the default eviction predicate and max idle/life time, but for HTTP/2 you keep them. Why do you introduce this difference?

I'll get this fixed over the weekend

@samueldlightfoot
Copy link
Contributor Author

samueldlightfoot commented Nov 19, 2022 •
edited

@violetagg Do we want to keep separate checks for idle and max lifetime or can we integrate into the eviction predicate like SimpleDequePool?

I have currently integrated max lifetime and idle time into the eviction predicate, but I just saw the Javadoc in the Class header saying we cannot use the eviction predicate from config due to more complexity. I think we should be fine to use the evictionPredicate but just with the constraint that active streams must also be 0 if the connection is open. Let me know what you think.

@samueldlightfoot samueldlightfoot force-pushed the eviction-predicate branch 7 times, most recently from b9b8826 to f1a1ea8 Compare November 20, 2022 13:06
@violetagg
Copy link
Member

@samueldlightfoot Is this ready for a review?

@samueldlightfoot
Copy link
Contributor Author

@samueldlightfoot Is this ready for a review?

Yes please

violetagg
violetagg requested changes Nov 22, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

Nice PR! Thanks!

I have just two small things which are not related to the functionality.

reactor-netty-core/src/main/java/reactor/netty/resources/ConnectionProvider.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/client/Http2PoolTest.java Outdated Show resolved Hide resolved
@samueldlightfoot
Copy link
Contributor Author

Committed both changes - thanks for the review. :-)

violetagg
violetagg approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

@reactor/netty-team PTAL

@violetagg violetagg requested a review from a team November 22, 2022 09:59
@violetagg violetagg added the type/enhancement A general enhancement label Nov 22, 2022
@violetagg violetagg added this to the 1.0.26 milestone Nov 22, 2022
pderop
pderop approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@pderop pderop left a comment

Choose a reason for hiding this comment

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

LGTM

violetagg pushed a commit that referenced this pull request Nov 22, 2022
@samueldlightfoot @violetagg
Allow configurable connection provider eviction predicate (#2557)
c8e30ba
violetagg added a commit that referenced this pull request Nov 22, 2022
@violetagg
Merge #2557 into 1.1.1
3b00086
violetagg added a commit that referenced this pull request Nov 22, 2022
@violetagg
Merge #2557 into 2.0.0-M4
adef851
@violetagg
Copy link
Member

violetagg commented Nov 22, 2022 •
edited

@samueldlightfoot I merged this in 1.0.x branch (c8e30ba) and then forward merged into main (3b00086) and netty5 (adef851)

@violetagg violetagg closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pderop pderop pderop approved these changes

@violetagg violetagg violetagg approved these changes

Assignees
No one assigned
Labels
type/enhancement A general enhancement
Projects
None yet
Milestone
1.0.26
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@samueldlightfoot @violetagg @pderop