loadbalancer-experimental: javadoc cleanup and minor fixes #3138
Conversation
Minor adjustments to indentation, validation, formatting, in code and javadoc.
...lk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/BaseHostSelector.java
Outdated
Show resolved
Hide resolved
...talk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java
Show resolved
Hide resolved
| @@ -17,6 +17,9 @@ | |||
|
|
|||
| import io.servicetalk.client.api.LoadBalancedConnection; | |||
|
|
|||
| /** | |||
| * A factory to create different {@link ConnectionPoolPolicy} variants. | |||
| */ | |||
| public final class ConnectionPoolPolicies { | |||
There was a problem hiding this comment.
Open thread for naming discussion (disclaimer: naming is hard):
The way currently javadoc is written around ConnectionPoolPolicy is that it's responsible for selection logic, not really for "pooling". It might be slightly more aligned if we rename it to ConnectionSelectionPolicy or ConnectionSelectorPolicy. That will highlight that it is only responsible for selecting from the pool (but not how to pool bcz it may imply internal data structure) and the fact it returns ConnectionSelector.
On the other hand, we can keep it as-is for now and rename when we are ready to introduce a public interface instead of an abstract class that we will be able to deprecate. So, if you promise me we will go from abstract class to an interface later, I would like to save new name proposal for the future 😄
WDYT?
There was a problem hiding this comment.
I think what you're proposing is that if we keep the abstract class and introduce an interface later we can use the good name (ConnectionSelectionPolicy) for the interface. Eg, not trying to convert the abstract class to an interface which I'm not sure is considered API compatible. Maybe it is since there is no way for users to create one, but otoh I'm not sure.
There was a problem hiding this comment.
You are right, if we don't allow users create their own implementations there is almost 0 risk to break them when we change from an abstract class to an interface. My only worry is that if someone releases a library that references ConnectionPoolPolicy and assigns it to a variable, will that library work without recompilation with the next version of ST if we change it to an interface? I don't remember binary compatibility rules in this case.
| @@ -16,7 +16,7 @@ | |||
| package io.servicetalk.loadbalancer; | |||
|
|
|||
| /** | |||
| * A collections of factories for constructing a {@link LoadBalancingPolicy}. | |||
| * A factory to create different {@link LoadBalancingPolicy} variants. | |||
| */ | |||
| public final class LoadBalancingPolicies { | |||
There was a problem hiding this comment.
Similar thought on renaming LoadBalancingPolicy. However here I really like the current name and don't have good ideas for a new name when we are ready to go from an abstract class to an interface.
...oadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Show resolved
Hide resolved
...oadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Show resolved
Hide resolved
...oadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Show resolved
Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicies.java
Show resolved
Hide resolved
...oadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Show resolved
Hide resolved
...oadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerObserver.java
Show resolved
Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancingPolicies.java
Show resolved
Hide resolved
...talk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java
Show resolved
Hide resolved
...talk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RequestTracker.java
Outdated
Show resolved
Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java
Outdated
Show resolved
Hide resolved
| @@ -17,6 +17,9 @@ | |||
|
|
|||
| import io.servicetalk.client.api.LoadBalancedConnection; | |||
|
|
|||
| /** | |||
| * A factory to create different {@link ConnectionPoolPolicy} variants. | |||
| */ | |||
| public final class ConnectionPoolPolicies { | |||
There was a problem hiding this comment.
I think what you're proposing is that if we keep the abstract class and introduce an interface later we can use the good name (ConnectionSelectionPolicy) for the interface. Eg, not trying to convert the abstract class to an interface which I'm not sure is considered API compatible. Maybe it is since there is no way for users to create one, but otoh I'm not sure.
Minor adjustments to indentation, validation, formatting, in code and javadoc.
Also reduced visibility where it was possible. Even if those methods are currently in pkg-private classes, it may help to start with minimal visibility on methods to avoid accidental API leaks when we make classes public in the future.