Adjusting ServiceDiscovererEvent contract
#1906
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
chemicL
requested review from
bondolo,
tkountis,
idelpivnitskiy and
Scottmitch
Motivation: Current API of `ServiceDiscovererEvent` is capable of representing the boolean status of the host being either available or not available. As seen in the implementation of RoundRobinLoadBalancer`, the resulting states from the Service Discovery system can have more meanings than a simple boolean. To prepare for enhanecements, the contract has been adjusted to allow more states and make it possible for further additions if those are needed by particular implementations. Modifications: - Deprecated `ServiceDiscovererEvent#isAvailable` method and added a default implementation to allow removal in child classes, - Added `ServiceDiscoveryStatus` class, - Added `ServiceDiscovererEvent#status` method with default implementation to prevent breaking the existing implementations, - Replaced existing uses of `isAvailable` with comparisons of `status` against the constants in `ServiceDiscoveryStatus`, - Added TODOs in order to reflect the need to adapt the codebase to support non-binary statuses. Result: `ServiceDiscovererEvent`s are prepared for more advanced, non-binary statuses. An example is the now introduced `EXPIRED` event, which can have a different meaning than `UNAVAILABLE`. If the `ServiceDiscoverer` decides to emit such a status, the subscribers can provide additional functionality in that case.
chemicL
force-pushed
the
service-discoverer-event-more-states
branch
from
250f6aa to
7946f9f
Compare
chemicL
commented
Oct 22, 2021
servicetalk-client-api/src/main/java/io/servicetalk/client/api/ServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
chemicL
added
the
breaking-change
idelpivnitskiy
requested changes
Oct 22, 2021
| @@ -104,17 +107,18 @@ private ServiceDiscovererUtils() { | |||
| private static <T> List<ServiceDiscovererEvent<T>> relativeComplement( | |||
There was a problem hiding this comment.
In 0.42 we should move the whole ServiceDiscovererUtils to dns module as pkg-private. Not sure why it had to be in client-api-internal as public.
...internal/src/main/java/io/servicetalk/client/api/internal/DefaultPartitionedClientGroup.java
Outdated
Show resolved
Hide resolved
...cetalk-client-api/src/main/java/io/servicetalk/client/api/DefaultServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
...cetalk-client-api/src/main/java/io/servicetalk/client/api/DefaultServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
...cetalk-client-api/src/main/java/io/servicetalk/client/api/DefaultServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
...k-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java
Outdated
Show resolved
Hide resolved
...ery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DnsServiceDiscovererObserver.java
Outdated
Show resolved
Hide resolved
...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java
Show resolved
Hide resolved
...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
chemicL
force-pushed
the
service-discoverer-event-more-states
branch
from
37d69db to
952842c
Compare
servicetalk-client-api/src/main/java/io/servicetalk/client/api/ServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
servicetalk-client-api/src/main/java/io/servicetalk/client/api/ServiceDiscoveryStatus.java
Outdated
Show resolved
Hide resolved
...nt-api-internal/src/main/java/io/servicetalk/client/api/internal/ServiceDiscovererUtils.java
Outdated
Show resolved
Hide resolved
...k-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java
Outdated
Show resolved
Hide resolved
...ery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DnsServiceDiscovererObserver.java
Outdated
Show resolved
Hide resolved
...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
|
Seems the build failure might be related to caching, as the error should not appear with the current state of the file mentioned. |
There was a problem hiding this comment.
The PR is marked as breaking-change, but is expected to land in 0.41. I don't see any breaking changes after the last comments will be addressed. Consider running scripts/japicmp.sh to make sure and remove the label.
LGTM after comments addressed. Feel free to ping if you want me to double-check before merging.
servicetalk-client-api/src/main/java/io/servicetalk/client/api/ServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
servicetalk-client-api/src/main/java/io/servicetalk/client/api/ServiceDiscovererEvent.java
Outdated
Show resolved
Hide resolved
...tty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java
Outdated
Show resolved
Hide resolved
...k-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java
Outdated
Show resolved
Hide resolved
...k-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java
Outdated
Show resolved
Hide resolved
...alk-http-api/src/main/java/io/servicetalk/http/api/DefaultServiceDiscoveryRetryStrategy.java
Outdated
Show resolved
Hide resolved
...s-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java
Outdated
Show resolved
Hide resolved
...lk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerFactory.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
...ery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DnsServiceDiscovererObserver.java
Outdated
Show resolved
Hide resolved
As a matter of fact, it seems to fail only in case of Java 8. Possibly caused by https://bugs.openjdk.java.net/browse/JDK-8031625. I submitted a follow up using the fully qualified name and that fixes the issue. |
chemicL
removed
the
breaking-change
...lk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerFactory.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
...lk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancerFactory.java
Outdated
Show resolved
Hide resolved
idelpivnitskiy
approved these changes
Nov 13, 2021
chemicL
pushed a commit
that referenced
this pull request
Nov 15, 2021
Motivation: Current API of `ServiceDiscovererEvent` is capable of representing the boolean status of the host being either available or not available. As seen in the implementation of RoundRobinLoadBalancer`, the resulting states from the Service Discovery system can have more meanings than a simple boolean. To prepare for enhanecements, the contract has been adjusted to allow more states and make it possible for further additions if those are needed by particular implementations. Modifications: - Deprecated `ServiceDiscovererEvent#isAvailable` method and added a default implementation to allow removal in child classes, - Added `ServiceDiscovererEvent.Status` class with `AVAILABLE`, `EXPIRED`, and `UNAVAILABLE` statuses, - Added `ServiceDiscovererEvent#status` method with default implementation to prevent breaking the existing implementations, - Replaced existing uses of `isAvailable` with comparisons of `status` against the constants in `ServiceDiscoveryStatus`, - `DefaultDnsClient` is capable of returning either of `UNAVAILABLE` or `EXPIRED` statuses for missing records, according to setting in `DefaultDnsServiceDiscovererBuilder#missingRecordStatus`. - `RoundRobinLoadBalancer` is able to act accordingly to received status, - `RoundRobinLoadBalancer#eagerConnectionShutdown` setting has been adjusted to allow no influence over received status and provides functionality when necessary for mapping `UNAVAILABLE` to `EXPIRED` and vice-versa. Result: `ServiceDiscovererEvent`s are prepared for more advanced, non-binary statuses. An example is the now introduced `EXPIRED` event, which has a different meaning than `UNAVAILABLE`. If the `ServiceDiscoverer` decides to emit such a status, the subscribers can provide additional functionality in that case such as the behavior provided by `RoundRobinLoadBalancer`.
|
0.41: ad81b3f |
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.
Motivation:
Current API of
ServiceDiscovererEventis capable of representing theboolean status of the host being either available or not available. As
seen in the implementation of
RoundRobinLoadBalancer, the resultingstates from the Service Discovery system can have more meanings than a
simple boolean. To prepare for enhanecements, the contract has been
adjusted to allow more states and make it possible for further additions
if those are needed by particular implementations.
Modifications:
ServiceDiscovererEvent#isAvailablemethod and added adefault implementation to allow removal in child classes,
ServiceDiscoveryStatusclass,ServiceDiscovererEvent#statusmethod with defaultimplementation to prevent breaking the existing implementations,
isAvailablewith comparisons ofstatusagainst the constants in
ServiceDiscoveryStatus,support non-binary statuses.
Result:
ServiceDiscovererEvents are prepared for more advanced, non-binarystatuses. An example is the now introduced
EXPIREDevent, which can have adifferent meaning than
UNAVAILABLE. If theServiceDiscovererdecidesto emit such a status, the subscribers can provide additional
functionality in that case.