xds: (minor) address Java Code Clarity recommendations in CSDS (#10804)
Redoes https://github.com/grpc/grpc-java/pull/8028
Ref cl/365582959
diff --git a/xds/src/main/java/io/grpc/xds/CsdsService.java b/xds/src/main/java/io/grpc/xds/CsdsService.java
index 3aab66d..20f78c1 100644
--- a/xds/src/main/java/io/grpc/xds/CsdsService.java
+++ b/xds/src/main/java/io/grpc/xds/CsdsService.java
@@ -80,6 +80,8 @@
if (handleRequest(request, responseObserver)) {
responseObserver.onCompleted();
}
+ // TODO(sergiitk): Add a case covering mutating handleRequest return false to true - to verify
+ // that responseObserver.onCompleted() isn't erroneously called on error.
}
@Override
@@ -115,12 +117,11 @@
Thread.currentThread().interrupt();
logger.log(Level.FINE, "Server interrupted while building CSDS config dump", e);
error = Status.ABORTED.withDescription("Thread interrupted").withCause(e).asException();
- } catch (Exception e) {
+ } catch (RuntimeException e) {
logger.log(Level.WARNING, "Unexpected error while building CSDS config dump", e);
error =
Status.INTERNAL.withDescription("Unexpected internal error").withCause(e).asException();
}
-
responseObserver.onError(error);
return false;
}
diff --git a/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java b/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java
index 6892324..904bcdc 100644
--- a/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java
+++ b/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java
@@ -37,6 +37,7 @@
import io.envoyproxy.envoy.service.status.v3.ClientStatusRequest;
import io.envoyproxy.envoy.service.status.v3.ClientStatusResponse;
import io.envoyproxy.envoy.type.matcher.v3.NodeMatcher;
+import io.grpc.Deadline;
import io.grpc.InsecureChannelCredentials;
import io.grpc.Status;
import io.grpc.Status.Code;
@@ -54,7 +55,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.junit.Before;
import org.junit.Rule;
@@ -97,7 +98,11 @@
@Before
public void setUp() {
- csdsStub = ClientStatusDiscoveryServiceGrpc.newBlockingStub(grpcServerRule.getChannel());
+ // The deadline is needed to prevent CsdsService#handleRequest mutation tests from hanging,
+ // because true->false return mutation prevents fetchClientStatus from completing the request.
+ csdsStub = ClientStatusDiscoveryServiceGrpc
+ .newBlockingStub(grpcServerRule.getChannel())
+ .withDeadline(Deadline.after(3, TimeUnit.SECONDS));
csdsAsyncStub = ClientStatusDiscoveryServiceGrpc.newStub(grpcServerRule.getChannel());
}
@@ -152,14 +157,10 @@
@Override
ListenableFuture<Map<XdsResourceType<?>, Map<String, ResourceMetadata>>>
getSubscribedResourcesMetadataSnapshot() {
- return Futures.submit(
- new Callable<Map<XdsResourceType<?>, Map<String, ResourceMetadata>>>() {
- @Override
- public Map<XdsResourceType<?>, Map<String, ResourceMetadata>> call() {
- Thread.currentThread().interrupt();
- return null;
- }
- }, MoreExecutors.directExecutor());
+ return Futures.submit(() -> {
+ Thread.currentThread().interrupt();
+ return null;
+ }, MoreExecutors.directExecutor());
}
};
grpcServerRule.getServiceRegistry()
@@ -214,7 +215,7 @@
requestObserver.onCompleted();
List<ClientStatusResponse> responses = responseObserver.getValues();
- assertThat(responses.size()).isEqualTo(3);
+ assertThat(responses).hasSize(3);
// Empty response on XdsClient not ready.
assertThat(responses.get(0)).isEqualTo(ClientStatusResponse.getDefaultInstance());
// The following calls return ClientConfig's successfully.
@@ -236,7 +237,7 @@
requestObserver.onCompleted();
List<ClientStatusResponse> responses = responseObserver.getValues();
- assertThat(responses.size()).isEqualTo(1);
+ assertThat(responses).hasSize(1);
verifyResponse(responses.get(0));
assertThat(responseObserver.getError()).isNotNull();
verifyRequestInvalidResponseStatus(Status.fromThrowable(responseObserver.getError()));
@@ -254,7 +255,7 @@
requestObserver.onError(new StatusRuntimeException(Status.DATA_LOSS));
List<ClientStatusResponse> responses = responseObserver.getValues();
- assertThat(responses.size()).isEqualTo(1);
+ assertThat(responses).hasSize(1);
verifyResponse(responses.get(0));
// Server quietly closes its side.
assertThat(responseObserver.getError()).isNull();
