-
Notifications
You must be signed in to change notification settings - Fork 3.9k
api: Javadoc changes for io.grpc.LoadBalancer method signatures #11623
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
ab97045
fef4c92
778cfb4
328bcbf
6a66054
4113845
09c3509
62a88ec
f658685
c51a5e4
02e92c5
4c31880
9ddcb09
596d11d
d9bf85e
401a7d2
f388ac2
6e4efc7
cb315f0
18b74c0
380345c
846eb00
ee36c4e
b1f857f
d236b88
891b547
6d3abf0
ca3e613
f9ffbc6
47002fe
cb85563
b44e402
8bfedb0
b9cced0
66a3495
45d3c75
b67b7ab
1b0ddaa
cfaab92
a0e5bf2
35f0769
bddc31c
b9a49a0
1e6fef9
1674b6d
60b4a39
8b02cd4
5ece923
d746ef8
516e75b
36ecadb
aa43806
d756690
d2e6f04
2ab1ae2
4a690a2
f66c084
8c75ed0
60932d8
4b542f7
9fc5391
af00e50
ea42960
226301c
e6bea7f
fd5263f
67c504f
29fd776
4300013
536311d
1ada412
9a63a50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,9 +29,21 @@ public abstract class ForwardingLoadBalancer extends LoadBalancer { | |
*/ | ||
protected abstract LoadBalancer delegate(); | ||
|
||
@Override | ||
/** | ||
* Handles newly resolved addresses and metadata attributes from name resolution system. | ||
* | ||
* @deprecated As of release 1.69.0, | ||
* use instead {@link #acceptResolvedAddresses(ResolvedAddresses)} | ||
*/ | ||
@Deprecated | ||
@SuppressWarnings("all") | ||
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will need some more thought. This change would prevent extending classes from seeing the addresses, if they are only overriding handleResolvedAddresses(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we mark this existing method as deprecated and introduce an override for acceptResolvedAddresses, please confirm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would still break an implementation only implementing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marking Something like,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shivaspeaks After removing delegate().acceptResolvedAddresses() from deprecated handleResolvedAddresses() method we are getting the PR checked failure with below compilation error warning: [InlineMeSuggester] This deprecated API looks inlineable. If you'd like the body of the API to be automatically inlined to its callers, please annotate it with @InlineMe. NOTE: the suggested fix makes the method final if it was not already. For more details please refer to the PR logs: https://github.com/grpc/grpc-java/actions/runs/13436677696/job/37540488347?pr=11623 Looking into this issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shivaspeaks Attempting to use the @InlineMe annotation did not resolve the error as it mandates the method to be marked as final which is unsuitable, suppressing all warnings also failed to address the issue. Anything you would like to suggest here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert this method's implementation back to forwarding to |
||
delegate().handleResolvedAddresses(resolvedAddresses); | ||
acceptResolvedAddresses(resolvedAddresses); | ||
} | ||
|
||
@Override | ||
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
return delegate().acceptResolvedAddresses(resolvedAddresses); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,13 @@ | |
@NotThreadSafe // Must be accessed in SynchronizationContext | ||
public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer { | ||
private final LoadBalancer defaultBalancer = new LoadBalancer() { | ||
/** | ||
* Handles newly resolved addresses and metadata attributes from name resolution system. | ||
* | ||
* @deprecated As of release 1.69.0, | ||
* use instead {@link #acceptResolvedAddresses(ResolvedAddresses)} | ||
*/ | ||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is not deprecated, and should not be marked deprecated. |
||
@Override | ||
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
throw new AssertionError("real LB is called instead"); | ||
|
@@ -84,6 +91,13 @@ public GracefulSwitchLoadBalancer(Helper helper) { | |
this.helper = checkNotNull(helper, "helper"); | ||
} | ||
|
||
/** | ||
* Handles newly resolved addresses and metadata attributes from name resolution system. | ||
* | ||
* @deprecated As of release 1.69.0, | ||
* use instead {@link #acceptResolvedAddresses(ResolvedAddresses)} | ||
*/ | ||
@Deprecated | ||
@Override | ||
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
ejona86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Config config = (Config) resolvedAddresses.getLoadBalancingPolicyConfig(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,8 @@ public void allMethodsForwarded() throws Exception { | |
mockDelegate, | ||
new TestBalancer(), | ||
Arrays.asList( | ||
LoadBalancer.class.getMethod("acceptResolvedAddresses", ResolvedAddresses.class))); | ||
LoadBalancer.class.getMethod("acceptResolvedAddresses", ResolvedAddresses.class), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. acceptResolvedAddresses should be removed already with the current code. After you change handleResolverAddresses to forward, it should be removed as well. |
||
LoadBalancer.class.getMethod("handleResolvedAddresses", ResolvedAddresses.class)) | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ public void tearDown() { | |
assertThat(fakeClock.getPendingTasks()).isEmpty(); | ||
} | ||
|
||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert these changes? The tests were converted to acceptResolvedAddresses. |
||
@Test | ||
public void acceptResolvedAddresses() { | ||
SocketAddress socketAddress = new InetSocketAddress(8080); | ||
|
@@ -239,6 +240,7 @@ public void acceptResolvedAddresses() { | |
verify(barBalancer0, never()).shutdown(); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void acceptResolvedAddresses_propagatesChildFailures() { | ||
LoadBalancerProvider lbProvider = new CannedLoadBalancer.Provider(); | ||
|
@@ -332,6 +334,7 @@ public void handleNameResolutionError() { | |
verify(fooLb1).handleNameResolutionError(status); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void typicalPriorityFailOverFlow() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -470,6 +473,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) { | |
verify(balancer3).shutdown(); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void idleToConnectingDoesNotTriggerFailOver() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -506,6 +510,7 @@ public void idleToConnectingDoesNotTriggerFailOver() { | |
assertThat(fooHelpers).hasSize(1); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void connectingResetFailOverIfSeenReadyOrIdleSinceTransientFailure() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -547,6 +552,7 @@ public void connectingResetFailOverIfSeenReadyOrIdleSinceTransientFailure() { | |
assertThat(fooHelpers).hasSize(2); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void readyToConnectDoesNotFailOverButUpdatesPicker() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -604,6 +610,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) { | |
assertThat(fooHelpers).hasSize(1); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void typicalPriorityFailOverFlowWithIdleUpdate() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -712,6 +719,7 @@ public void typicalPriorityFailOverFlowWithIdleUpdate() { | |
verify(balancer3).shutdown(); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void failover_propagatesChildFailures() { | ||
LoadBalancerProvider lbProvider = new CannedLoadBalancer.Provider(); | ||
|
@@ -789,6 +797,7 @@ public void bypassReresolutionRequestsIfConfiged() { | |
verify(helper).refreshNameResolution(); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void raceBetweenShutdownAndChildLbBalancingStateUpdate() { | ||
PriorityChildConfig priorityChildConfig0 = | ||
|
@@ -814,6 +823,7 @@ public void raceBetweenShutdownAndChildLbBalancingStateUpdate() { | |
verifyNoMoreInteractions(helper); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void noDuplicateOverallBalancingStateUpdate() { | ||
FakeLoadBalancerProvider fakeLbProvider = new FakeLoadBalancerProvider(); | ||
|
@@ -914,6 +924,7 @@ static class FakeLoadBalancer extends LoadBalancer { | |
this.helper = helper; | ||
} | ||
|
||
@Deprecated | ||
@Override | ||
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
helper.updateBalancingState( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,7 @@ public void tearDown() { | |
} | ||
} | ||
|
||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to fix these. I've sent out #12053 |
||
@Test | ||
public void handleResolvedAddresses() { | ||
ArgumentCaptor<ResolvedAddresses> resolvedAddressesCaptor = | ||
|
@@ -264,6 +265,7 @@ public void handleResolvedAddresses() { | |
} | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void handleNameResolutionError() { | ||
ArgumentCaptor<SubchannelPicker> pickerCaptor = ArgumentCaptor.forClass(SubchannelPicker.class); | ||
|
@@ -302,6 +304,7 @@ public void handleNameResolutionError() { | |
} | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void balancingStateUpdatedFromChildBalancers() { | ||
Map<String, WeightedPolicySelection> targets = ImmutableMap.of( | ||
|
@@ -390,6 +393,7 @@ public void balancingStateUpdatedFromChildBalancers() { | |
new WeightedChildPicker(weights[3], failurePickers[3])); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void raceBetweenShutdownAndChildLbBalancingStateUpdate() { | ||
Map<String, WeightedPolicySelection> targets = ImmutableMap.of( | ||
|
@@ -412,6 +416,7 @@ public void raceBetweenShutdownAndChildLbBalancingStateUpdate() { | |
|
||
// When the ChildHelper is asked to update the overall balancing state, it should not do that if | ||
// the update was triggered by the parent LB that will handle triggering the overall state update. | ||
@Deprecated | ||
@Test | ||
public void noDuplicateOverallBalancingStateUpdate() { | ||
FakeLoadBalancerProvider fakeLbProvider = new FakeLoadBalancerProvider(); | ||
|
@@ -469,6 +474,7 @@ static class FakeLoadBalancer extends LoadBalancer { | |
this.helper = helper; | ||
} | ||
|
||
@Deprecated | ||
@Override | ||
public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | ||
helper.updateBalancingState( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,7 @@ public void setUp() { | |
loadBalancer = new WrrLocalityLoadBalancer(mockHelper, lbRegistry); | ||
} | ||
|
||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert these changes? The tests were converted to acceptResolvedAddresses. |
||
@Test | ||
public void acceptResolvedAddresses() { | ||
// A two locality cluster with a mock child LB policy. | ||
|
@@ -173,6 +174,7 @@ public void handleNameResolutionError_withChildLb() { | |
verify(mockWeightedTargetLb).handleNameResolutionError(status); | ||
} | ||
|
||
@Deprecated | ||
@Test | ||
public void localityWeightAttributeNotPropagated() { | ||
Object childPolicy = newChildConfig(mockChildProvider, null); | ||
|
@@ -212,6 +214,7 @@ private Object newChildConfig(LoadBalancerProvider provider, Object config) { | |
return GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(provider, config); | ||
} | ||
|
||
@Deprecated | ||
private void deliverAddresses(WrrLocalityConfig config, List<EquivalentAddressGroup> addresses) { | ||
loadBalancer.acceptResolvedAddresses( | ||
ResolvedAddresses.newBuilder().setAddresses(addresses).setLoadBalancingPolicyConfig(config) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a javadoc on Deprecated and what to use instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included in Javadoc