Skip to content

xds: fix style #6834

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

Merged
creamsoup merged 1 commit into from
Mar 16, 2020
Merged

xds: fix style #6834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/WeightedTargetLoadBalancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ private void updateOverallBalancingState() {
}

@Nullable
private ConnectivityState aggregateState(
private static ConnectivityState aggregateState(
@Nullable ConnectivityState overallState, ConnectivityState childState) {
if (overallState == null) {
return childState;
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/WeightedTargetLoadBalancerProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Objects.hash(targets);
return Objects.hashCode(targets);

@voidzcy voidzcy Mar 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am confused. In @dapengzhang0 's recent PR, he explicitly changed usages of Objects.hashCode() to Objects.hash(). So which one on earth should we use?

@creamsoup creamsoup Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

according to the linter both generates same value. but hash accepts vararg, so it creates unnecessary objects when calling this method. the object is not really used in the hash based collection so it doesn't really matter tbh.

@dapengzhang0 dapengzhang0 Mar 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The recent PR was changing from guava Objects to java.util.Objects. It's a different issue.

}

@Override
Expand Down
8 changes: 4 additions & 4 deletions xds/src/main/java/io/grpc/xds/XdsClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ final class XdsClientImpl extends XdsClient {
"type.googleapis.com/envoy.api.v2.ClusterLoadAssignment";

// For now we do not support path matching unless enabled manually.
private static boolean enablePathMatching = Boolean.parseBoolean(
private static final boolean ENABLE_PATH_MATCHING = Boolean.parseBoolean(
System.getenv("ENABLE_EXPERIMENTAL_PATH_MATCHING"));

private final MessagePrinter respPrinter = new MessagePrinter();
Expand Down Expand Up @@ -641,7 +641,7 @@ private void handleLdsResponseForConfigUpdate(DiscoveryResponse ldsResponse) {
if (routes != null) {
// Found clusterName in the in-lined RouteConfiguration.
String clusterName = routes.get(routes.size() - 1).getRouteAction().get().getCluster();
if (!enablePathMatching) {
if (!ENABLE_PATH_MATCHING) {
logger.log(
XdsLogLevel.INFO,
"Found cluster name (inlined in route config): {0}", clusterName);
Expand Down Expand Up @@ -812,7 +812,7 @@ private void handleRdsResponse(DiscoveryResponse rdsResponse) {

// Found clusterName in the in-lined RouteConfiguration.
String clusterName = routes.get(routes.size() - 1).getRouteAction().get().getCluster();
if (!enablePathMatching) {
if (!ENABLE_PATH_MATCHING) {
logger.log(XdsLogLevel.INFO, "Found cluster name: {0}", clusterName);
} else {
logger.log(XdsLogLevel.INFO, "Found {0} routes", routes.size());
Expand Down Expand Up @@ -893,7 +893,7 @@ private static String validateRoutes(List<EnvoyProtoData.Route> routes) {
}

// We only validate the default route unless path matching is enabled.
if (!enablePathMatching) {
if (!ENABLE_PATH_MATCHING) {
EnvoyProtoData.Route route = routes.get(routes.size() - 1);
RouteMatch routeMatch = route.getRouteMatch();
if (!routeMatch.getPath().isEmpty() || !routeMatch.getPrefix().isEmpty()
Expand Down
9 changes: 3 additions & 6 deletions xds/src/test/java/io/grpc/xds/LocalityStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,8 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
assertThat(interLocalityPicker.weightedChildPickers).hasSize(2);

Set<Subchannel> pickedReadySubchannels = new HashSet<>();
for (int i = 0; i < interLocalityPicker.weightedChildPickers.size(); i++) {
PickResult result = interLocalityPicker.weightedChildPickers.get(i).getPicker()
.pickSubchannel(pickSubchannelArgs);
for (WeightedChildPicker weightedPicker : interLocalityPicker.weightedChildPickers) {
PickResult result = weightedPicker.getPicker().pickSubchannel(pickSubchannelArgs);
pickedReadySubchannels.add(result.getSubchannel());
}
assertThat(pickedReadySubchannels).containsExactly(subchannel31, subchannel12);
Expand Down Expand Up @@ -916,9 +915,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
WeightedRandomPicker interLocalityPicker =
(WeightedRandomPicker) subchannelPickerCaptor.getValue();
assertThat(interLocalityPicker.weightedChildPickers).hasSize(3);
for (int i = 0; i < interLocalityPicker.weightedChildPickers.size(); i++) {
WeightedChildPicker weightedChildPicker
= interLocalityPicker.weightedChildPickers.get(i);
for (WeightedChildPicker weightedChildPicker : interLocalityPicker.weightedChildPickers) {
Subchannel subchannel
= weightedChildPicker.getPicker().pickSubchannel(pickSubchannelArgs).getSubchannel();
assertThat(weightedChildPicker.getWeight())
Expand Down
2 changes: 1 addition & 1 deletion xds/src/test/java/io/grpc/xds/WeightedTargetLoadBalancerProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
+ " ]"
+ " }"
+ " }"
+ "}").replace("'", "\"");
+ "}").replace('\'', '"');

@SuppressWarnings("unchecked")
Map<String, ?> rawLbConfigMap = (Map<String, ?>) JsonParser.parse(weightedTargetConfigJson);
Expand Down
22 changes: 11 additions & 11 deletions xds/src/test/java/io/grpc/xds/WeightedTargetLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,36 +305,36 @@ public void balancingStateUpdatedFromChildBalancers() {
verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture());
assertThat(pickerCaptor.getValue()).isInstanceOf(WeightedRandomPicker.class);
WeightedRandomPicker overallPicker = (WeightedRandomPicker) pickerCaptor.getValue();
assertThat(overallPicker.weightedChildPickers).isEqualTo(
ImmutableList.of(new WeightedChildPicker(weights[2], subchannelPickers[2])));
assertThat(overallPicker.weightedChildPickers)
.containsExactly(new WeightedChildPicker(weights[2], subchannelPickers[2]));

// Another child balancer goes to READY.
childHelpers.get(3).updateBalancingState(READY, subchannelPickers[3]);
verify(helper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture());
overallPicker = (WeightedRandomPicker) pickerCaptor.getValue();
assertThat(overallPicker.weightedChildPickers).isEqualTo(
ImmutableList.of(
assertThat(overallPicker.weightedChildPickers)
.containsExactly(
new WeightedChildPicker(weights[2], subchannelPickers[2]),
new WeightedChildPicker(weights[3], subchannelPickers[3])));
new WeightedChildPicker(weights[3], subchannelPickers[3]));

// Another child balancer goes to READY.
childHelpers.get(0).updateBalancingState(READY, subchannelPickers[0]);
verify(helper, times(3)).updateBalancingState(eq(READY), pickerCaptor.capture());
overallPicker = (WeightedRandomPicker) pickerCaptor.getValue();
assertThat(overallPicker.weightedChildPickers).isEqualTo(
ImmutableList.of(
assertThat(overallPicker.weightedChildPickers)
.containsExactly(
new WeightedChildPicker(weights[0], subchannelPickers[0]),
new WeightedChildPicker(weights[2], subchannelPickers[2]),
new WeightedChildPicker(weights[3], subchannelPickers[3])));
new WeightedChildPicker(weights[3], subchannelPickers[3]));

// One of READY child balancers goes to TRANSIENT_FAILURE.
childHelpers.get(2).updateBalancingState(TRANSIENT_FAILURE, new ErrorPicker(Status.DATA_LOSS));
verify(helper, times(4)).updateBalancingState(eq(READY), pickerCaptor.capture());
overallPicker = (WeightedRandomPicker) pickerCaptor.getValue();
assertThat(overallPicker.weightedChildPickers).isEqualTo(
ImmutableList.of(
assertThat(overallPicker.weightedChildPickers)
.containsExactly(
new WeightedChildPicker(weights[0], subchannelPickers[0]),
new WeightedChildPicker(weights[3], subchannelPickers[3])));
new WeightedChildPicker(weights[3], subchannelPickers[3]));

// All child balancers go to TRANSIENT_FAILURE.
childHelpers.get(3).updateBalancingState(TRANSIENT_FAILURE, new ErrorPicker(Status.DATA_LOSS));
Expand Down
2 changes: 1 addition & 1 deletion xds/src/test/java/io/grpc/xds/XdsClientImplTestForListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public void tearDown() {
assertThat(fakeClock.getPendingTasks()).isEmpty();
}

private Node getNodeToVerify() {
private static Node getNodeToVerify() {
Struct newMetadata = NODE.getMetadata().toBuilder()
.putFields("listener_inbound_port",
Value.newBuilder().setStringValue("" + PORT).build())
Expand Down