Address the comment from @ikhoon

line · Mar 4, 2025 · 2e48dd9 · 2e48dd9
1 parent 2307750
commit 2e48dd9
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 57 deletions.
diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
@@ -438,18 +438,13 @@ private GrpcServiceBuilder addProtoReflectionService(boolean hasDecorators,
                    "Attempting to add a ProtoReflectionService but one is already present. " +
                    "ProtoReflectionService must only be added once.");
         protoReflectionServiceInterceptor = new ProtoReflectionServiceInterceptor();
+        if (protoReflectionService instanceof ProtoReflectionService) {
+            logger.warn("Use {} instead of {}.", ProtoReflectionServiceV1.class.getSimpleName(),
+                        protoReflectionService);
+        }
         final ServerServiceDefinition intercept =
                 ServerInterceptors.intercept(protoReflectionService, protoReflectionServiceInterceptor);
-        addProtoReflectionIntercept(intercept, path, methodDescriptor);
-        if (!(protoReflectionService instanceof ProtoReflectionService)) {
-            return this;
-        }
-        logger.warn("Use {} instead of {}.", ProtoReflectionServiceV1.class.getSimpleName(),
-                    protoReflectionService);
-        final ServerServiceDefinition interceptV1 =
-                ServerInterceptors.intercept(ProtoReflectionServiceV1.newInstance(),
-                                             protoReflectionServiceInterceptor);
-        return addProtoReflectionIntercept(interceptV1, path, methodDescriptor);
+        return addProtoReflectionIntercept(intercept, path, methodDescriptor);
     }
 
     private GrpcServiceBuilder addProtoReflectionIntercept(ServerServiceDefinition intercept,

diff --git a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceServerTest.java b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceServerTest.java
@@ -109,11 +109,11 @@
 import io.grpc.Status.Code;
 import io.grpc.StatusRuntimeException;
 import io.grpc.protobuf.ProtoUtils;
-import io.grpc.protobuf.services.ProtoReflectionService;
-import io.grpc.reflection.v1alpha.ServerReflectionGrpc;
-import io.grpc.reflection.v1alpha.ServerReflectionGrpc.ServerReflectionStub;
-import io.grpc.reflection.v1alpha.ServerReflectionRequest;
-import io.grpc.reflection.v1alpha.ServerReflectionResponse;
+import io.grpc.protobuf.services.ProtoReflectionServiceV1;
+import io.grpc.reflection.v1.ServerReflectionGrpc;
+import io.grpc.reflection.v1.ServerReflectionGrpc.ServerReflectionStub;
+import io.grpc.reflection.v1.ServerReflectionRequest;
+import io.grpc.reflection.v1.ServerReflectionResponse;
 import io.grpc.stub.ServerCallStreamObserver;
 import io.grpc.stub.StreamObserver;
 import io.netty.buffer.ByteBuf;
@@ -453,8 +453,7 @@ protected void configure(ServerBuilder sb) throws Exception {
 
             sb.service(
                     GrpcService.builder()
-                               // ProtoReflectionServiceV1 is added by default.
-                               .addService(ProtoReflectionService.newInstance())
+                               .addService(ProtoReflectionServiceV1.newInstance())
                                .build(),
                     service -> service.decorate(LoggingService.newDecorator()));
         }
@@ -1230,7 +1229,8 @@ void longMaxRequestLimit() throws Exception {
     void reflectionService() throws Exception {
         final ServerReflectionStub stub = ServerReflectionGrpc.newStub(channel);
 
-        final AtomicReference<ServerReflectionResponse> response = new AtomicReference<>();
+        final AtomicReference<ServerReflectionResponse> response =
+                new AtomicReference<>();
 
         final StreamObserver<ServerReflectionRequest> request = stub.serverReflectionInfo(
                 new StreamObserver<ServerReflectionResponse>() {
@@ -1261,43 +1261,6 @@ public void onCompleted() {}
                 });
     }
 
-    @Test
-    void reflectionServiceV1() throws Exception {
-        final io.grpc.reflection.v1.ServerReflectionGrpc.ServerReflectionStub stub =
-                io.grpc.reflection.v1.ServerReflectionGrpc.newStub(channel);
-
-        final AtomicReference<io.grpc.reflection.v1.ServerReflectionResponse> response =
-                new AtomicReference<>();
-
-        final StreamObserver<io.grpc.reflection.v1.ServerReflectionRequest> request = stub.serverReflectionInfo(
-                new StreamObserver<io.grpc.reflection.v1.ServerReflectionResponse>() {
-                    @Override
-                    public void onNext(io.grpc.reflection.v1.ServerReflectionResponse value) {
-                        response.set(value);
-                    }
-
-                    @Override
-                    public void onError(Throwable t) {}
-
-                    @Override
-                    public void onCompleted() {}
-                });
-        request.onNext(io.grpc.reflection.v1.ServerReflectionRequest.newBuilder()
-                                                                    .setListServices("")
-                                                                    .build());
-        request.onCompleted();
-
-        await().untilAsserted(
-                () -> {
-                    assertThat(response).doesNotHaveValue(null);
-                    // Instead of making this test depend on every other one, just check that there is at
-                    // least two services returned corresponding to UnitTestService and
-                    // ProtoReflectionService.
-                    assertThat(response.get().getListServicesResponse().getServiceList())
-                            .hasSizeGreaterThanOrEqualTo(2);
-                });
-    }
-
     @ParameterizedTest
     @ArgumentsSource(BlockingClientProvider.class)
     void replaceException(UnitTestServiceBlockingStub blockingClient) throws Exception {

diff --git a/...main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.java b/...main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.java
@@ -76,8 +76,6 @@ public void applyTo(Builder builder) {
         builder.allowBlockingCallsInside("com.linecorp.armeria.client.ClientFactory", "ofDefault");
         builder.allowBlockingCallsInside("io.envoyproxy.controlplane.cache.SimpleCache", "createWatch");
         builder.allowBlockingCallsInside("io.grpc.netty.shaded.io.netty.util.Version", "identify");
-        builder.allowBlockingCallsInside("io.fabric8.kubernetes.client.server.mock.WatchEventsListener",
-                                         "onClosed");
 
         // prints the exception which makes it easier to debug issues
         builder.blockingMethodCallback(this::writeBlockingMethod);