loadbalancer: Some cleanups for DefaultHost #2779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryce-anderson
requested review from
daschl,
mgodave,
tkountis and
idelpivnitskiy
daschl
reviewed
Dec 13, 2023
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Show resolved
Hide resolved
Motivation: DefaultHost has a bunch of state balled up in ConnState and we end up doing a bunch of unchecked casts. This makes it tough to read and tough for the compiler to do it's job. It turns out that we also have some poor life-cycle management going on so we can tidy that up as well. Modifications: Make ConnState an instance class and pull in the type arg C. Make it's connection array a List. This is because it's not easy to make an array from generic types which is perhaps the reason we were using an Object[] in the first place. Remove the Host.markClosed() method and instead just use the AsyncClosable close API's. This makes it so the onClose listeners all hear when the connection is closed, and not just due to expiration.
bryce-anderson
force-pushed
the
bl_anderson/DefaultHostCleanups
branch
from
4f9cddd to
8b34b75
Compare
bryce-anderson
force-pushed
the
bl_anderson/DefaultHostCleanups
branch
from
8b34b75 to
2b84fe3
Compare
tkountis
approved these changes
Dec 19, 2023
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultHost.java
Outdated
Show resolved
Hide resolved
daschl
reviewed
Dec 19, 2023
servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/DefaultHostTest.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: this is based on #2770 which I'd like to merge first.
Motivation:
DefaultHost has a bunch of state balled up in
ConnStateand we end up doing a bunch of unchecked casts. This makes
it tough to read and tough for the compiler to do it's job.
It turns out that we also have some poor life-cycle
management going on so we can tidy that up as well.
Modifications:
ConnStatean instance class and pull in the type arg C.not easy to make an array from generic types which is perhaps
the reason we were using an
Object[]in the first place.Host.markClosed()method and instead just usethe
AsyncClosableclose API's. This makes it so theonCloselisteners all hear when the connection is closed, and not just
due to expiration.