From 9e851f7406f0bb04bd1342272c5eb02e68854e12 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 26 Mar 2024 11:26:36 -0700 Subject: [PATCH] categorize todos --- xds/src/main/java/io/grpc/xds/Filter.java | 4 ++-- xds/src/main/java/io/grpc/xds/RlqsFilter.java | 12 ++++++------ .../main/java/io/grpc/xds/RlqsFilterConfig.java | 2 +- .../main/java/io/grpc/xds/XdsServerWrapper.java | 2 +- .../grpc/xds/internal/datatype/GrpcService.java | 16 ++++++++-------- .../io/grpc/xds/internal/rlqs/RlqsClient.java | 2 +- .../grpc/xds/internal/rlqs/RlqsClientPool.java | 7 +++---- 7 files changed, 22 insertions(+), 23 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/Filter.java b/xds/src/main/java/io/grpc/xds/Filter.java index 056a48e43bbf..9383f6df63a7 100644 --- a/xds/src/main/java/io/grpc/xds/Filter.java +++ b/xds/src/main/java/io/grpc/xds/Filter.java @@ -52,8 +52,8 @@ interface Filter { default void shutdown() { // Implement as needed. - // TODO(sergiitk): important to cover and discuss in the design. - // TODO(sergiitk): should it be in ServerInterceptorBuilder? + // TODO(sergiitk): [DESIGN] important to cover and discuss in the design. + // TODO(sergiitk): [QUESTION] should it be in ServerInterceptorBuilder? } /** Represents an opaque data structure holding configuration for a filter. */ diff --git a/xds/src/main/java/io/grpc/xds/RlqsFilter.java b/xds/src/main/java/io/grpc/xds/RlqsFilter.java index f5f4d12e152a..561b5091e45a 100644 --- a/xds/src/main/java/io/grpc/xds/RlqsFilter.java +++ b/xds/src/main/java/io/grpc/xds/RlqsFilter.java @@ -97,8 +97,7 @@ public ServerInterceptor buildServerInterceptor( FilterConfig config, @Nullable FilterConfig overrideConfig, ScheduledExecutorService scheduler) { - // called when we get an xds update - when the LRS or RLS changes. - // TODO(sergiitk): this needs to be confirmed. + // Called when we get an xds update - when the LRS or RLS changes. RlqsFilterConfig rlqsFilterConfig = (RlqsFilterConfig) checkNotNull(config, "config"); // Per-route and per-host configuration overrides. @@ -119,7 +118,8 @@ public ServerInterceptor buildServerInterceptor( @Override public void shutdown() { - // TODO(sergiitk): besides shutting down everything, should there be a per-route destructor? + // TODO(sergiitk): [DESIGN] besides shutting down everything, should there + // be per-route interceptor destructors? RlqsClientPool oldClientPool = rlqsClientPoolRef.getAndUpdate(unused -> null); if (oldClientPool != null) { oldClientPool.shutdown(); @@ -152,7 +152,7 @@ public Listener interceptCall( // - and interface to notify service interceptor on shutdown // - destroy channel when ref count 0 // potentially many RLQS Clients sharing a channel to grpc RLQS service - - // TODO(sergiitk): look up how cache is looked up + // TODO(sergiitk): [QUESTION] look up how cache is looked up // now we create filters every RPC. will be change in RBAC. // we need to avoid recreating filter when config doesn't change // m: trigger close() after we create new instances @@ -188,7 +188,7 @@ static RlqsFilterConfig parseRlqsFilter(RateLimitQuotaFilterConfig rlqsFilterPro builder.domain(rlqsFilterProto.getDomain()) .rlqsService(GrpcService.fromEnvoyProto(rlqsFilterProto.getRlqsServer())); - // TODO(sergiitk): bucket_matchers. + // TODO(sergiitk): [IMPL] bucket_matchers. return builder.build(); } @@ -197,7 +197,7 @@ static RlqsFilterConfig parseRlqsFilter(RateLimitQuotaFilterConfig rlqsFilterPro static RlqsFilterConfig parseRlqsFilterOverride(RateLimitQuotaOverride rlqsFilterProtoOverride) throws ResourceInvalidException { RlqsFilterConfig.Builder builder = RlqsFilterConfig.builder(); - // TODO(sergiitk): bucket_matchers. + // TODO(sergiitk): [IMPL] bucket_matchers. return builder.domain(rlqsFilterProtoOverride.getDomain()).build(); } diff --git a/xds/src/main/java/io/grpc/xds/RlqsFilterConfig.java b/xds/src/main/java/io/grpc/xds/RlqsFilterConfig.java index 809d179abf0d..fea71a4c54fb 100644 --- a/xds/src/main/java/io/grpc/xds/RlqsFilterConfig.java +++ b/xds/src/main/java/io/grpc/xds/RlqsFilterConfig.java @@ -50,5 +50,5 @@ abstract static class Builder { abstract RlqsFilterConfig build(); } - // TODO(sergiitk): add rlqs_server, bucket_matchers. + // TODO(sergiitk): [IMPL] add rlqs_server, bucket_matchers. } diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java index 26eddab30a19..0d20eec20004 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java @@ -455,7 +455,7 @@ private void shutdown() { private void updateSelector() { Map> filterChainRouting = new HashMap<>(); - // TODO(sergiitk): is this a good place to reset interceptors? + // TODO(sergiitk): [QUESTION] is this a good place to reset interceptors? // for (FilterChain filterChain : savedRdsRoutingConfigRef.keySet()) { // if (!resourceName.equals(filterChain.httpConnectionManager().rdsName())) { // continue; diff --git a/xds/src/main/java/io/grpc/xds/internal/datatype/GrpcService.java b/xds/src/main/java/io/grpc/xds/internal/datatype/GrpcService.java index 2336e20de602..658544a42916 100644 --- a/xds/src/main/java/io/grpc/xds/internal/datatype/GrpcService.java +++ b/xds/src/main/java/io/grpc/xds/internal/datatype/GrpcService.java @@ -26,12 +26,12 @@ public abstract class GrpcService { public abstract String targetUri(); - // TODO(sergiitk): do we need this? + // TODO(sergiitk): [QUESTION] do we need this? // abstract String statPrefix(); - // TODO(sergiitk): channelCredentials - // TODO(sergiitk): callCredentials - // TODO(sergiitk): channelArgs + // TODO(sergiitk): [IMPL] channelCredentials + // TODO(sergiitk): [IMPL] callCredentials + // TODO(sergiitk): [IMPL] channelArgs /** Optional timeout duration for the gRPC request to the service. */ @Nullable @@ -54,10 +54,10 @@ public static GrpcService fromEnvoyProto( grpcServiceProto.getGoogleGrpc(); builder.targetUri(googleGrpcProto.getTargetUri()); - // TODO(sergiitk): channelCredentials - // TODO(sergiitk): callCredentials - // TODO(sergiitk): channelArgs - // TODO(sergiitk): statPrefix - (maybe) + // TODO(sergiitk): [IMPL] channelCredentials + // TODO(sergiitk): [IMPL] callCredentials + // TODO(sergiitk): [IMPL] channelArgs + // TODO(sergiitk): [IMPL] statPrefix - (maybe) return builder.build(); } diff --git a/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClient.java b/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClient.java index b6faeecdad45..f8f639fb03db 100644 --- a/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClient.java +++ b/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClient.java @@ -31,6 +31,6 @@ final class RlqsClient { public void shutdown() { logger.log(Level.FINER, "Shutting down RlqsClient to {0}", targetUri); - // TODO(sergiitk): impl + // TODO(sergiitk): [IMPL] RlqsClient shutdown } } diff --git a/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClientPool.java b/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClientPool.java index 116f8e487f0d..5ac01329bfe4 100644 --- a/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClientPool.java +++ b/xds/src/main/java/io/grpc/xds/internal/rlqs/RlqsClientPool.java @@ -32,10 +32,9 @@ public final class RlqsClientPool { private static final Logger logger = Logger.getLogger(RlqsClientPool.class.getName()); - // TODO(sergiitk): make a param? private static final int DEFAULT_CLEANUP_INTERVAL_SECONDS = 10; - // TODO(sergiitk): always in sync context? + // TODO(sergiitk): [QUESTION] always in sync context? private boolean shutdown; private final SynchronizationContext syncContext = new SynchronizationContext((thread, error) -> { String message = "Uncaught exception in RlqsClientPool SynchronizationContext. Panic!"; @@ -58,8 +57,8 @@ private RlqsClientPool(ScheduledExecutorService scheduler, int cleanupIntervalSe /** Creates an instance. */ public static RlqsClientPool newInstance(ScheduledExecutorService scheduler) { - // TODO(sergiitk): scheduler - consider using GrpcUtil.TIMER_SERVICE. - // TODO(sergiitk): note that the scheduler has a finite lifetime. + // TODO(sergiitk): [IMPL] scheduler - consider using GrpcUtil.TIMER_SERVICE. + // TODO(sergiitk): [IMPL] note that the scheduler has a finite lifetime. return new RlqsClientPool(scheduler, 0); }