Addressing review comments: Made lookup reason always as cache miss after a backed off entry is reprocessed, and the style related comments. Added assertions for lookup reason in unit tests.

Author: kannanjgithub

rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java

@@ -128,7 +128,6 @@ final class CachingRlsLbClient {
   private final RlsLbHelper helper;
   private final ManagedChannel rlsChannel;
   private final RouteLookupServiceStub rlsStub;
-  private final RlsRequestFactory requestFactory;
   private final RlsPicker rlsPicker;
   private final ResolvedAddressFactory childLbResolvedAddressFactory;
   @GuardedBy("lock")
@@ -197,7 +196,7 @@ private CachingRlsLbClient(Builder builder) {
           ChannelLogLevel.DEBUG, "Can not get hostname from authority: {0}", helper.getAuthority());
       serverHost = helper.getAuthority();
     }
-    requestFactory = new RlsRequestFactory(
+    RlsRequestFactory requestFactory = new RlsRequestFactory(
         lbPolicyConfig.getRouteLookupConfig(), serverHost);
     rlsPicker = new RlsPicker(requestFactory, rlsConfig.lookupService());
     // It is safe to use helper.getUnsafeChannelCredentials() because the client authenticates the
@@ -303,7 +302,7 @@ private CachedRouteLookupResponse asyncRlsCall(
       // on this result
       return CachedRouteLookupResponse.backoffEntry(createBackOffEntry(
           routeLookupRequestKey, Status.RESOURCE_EXHAUSTED.withDescription("RLS throttled"),
-          backoffPolicy, routeLookupReason));
+          backoffPolicy));
     }
     final SettableFuture<RouteLookupResponse> response = SettableFuture.create();
     io.grpc.lookup.v1.RouteLookupRequest routeLookupRequest = REQUEST_CONVERTER.convert(
@@ -417,7 +416,7 @@ private void pendingRpcComplete(PendingCacheEntry entry) {
         // reattempt picks when the child LB is done connecting
       } catch (Exception e) {
         createBackOffEntry(entry.routeLookupRequestKey, Status.fromThrowable(e),
-            entry.backoffPolicy, entry.routeLookupReason);
+            entry.backoffPolicy);
         // Cache updated. updateBalancingState() to reattempt picks
         helper.triggerPendingRpcProcessing();
       }
@@ -439,9 +438,8 @@ private DataCacheEntry createDataEntry(
   }
 
   @GuardedBy("lock")
-  private BackoffCacheEntry createBackOffEntry(
-      RouteLookupRequestKey routeLookupRequestKey, Status status,
-      @Nullable BackoffPolicy backoffPolicy, RouteLookupRequest.Reason routeLookupReason) {
+  private BackoffCacheEntry createBackOffEntry(RouteLookupRequestKey routeLookupRequestKey,
+      Status status, @Nullable BackoffPolicy backoffPolicy) {
     if (backoffPolicy == null) {
       backoffPolicy = backoffProvider.get();
     }
@@ -450,8 +448,7 @@ private BackoffCacheEntry createBackOffEntry(
         ChannelLogLevel.DEBUG,
         "[RLS Entry {0}] Transition to back off: status={1}, delayNanos={2}",
         routeLookupRequestKey, status, delayNanos);
-    BackoffCacheEntry entry = new BackoffCacheEntry(routeLookupRequestKey, status, backoffPolicy,
-        routeLookupReason);
+    BackoffCacheEntry entry = new BackoffCacheEntry(routeLookupRequestKey, status, backoffPolicy);
     // Lock is held, so the task can't execute before the assignment
     entry.scheduledFuture = scheduledExecutorService.schedule(
         () -> refreshBackoffEntry(entry), delayNanos, TimeUnit.NANOSECONDS);
@@ -469,7 +466,7 @@ private void refreshBackoffEntry(BackoffCacheEntry entry) {
       logger.log(ChannelLogLevel.DEBUG,
           "[RLS Entry {0}] Calling RLS for transition to pending", entry.routeLookupRequestKey);
       linkedHashLruCache.invalidate(entry.routeLookupRequestKey);
-      asyncRlsCall(entry.routeLookupRequestKey, entry.backoffPolicy, entry.routeLookupReason);
+      asyncRlsCall(entry.routeLookupRequestKey, entry.backoffPolicy, RouteLookupRequest.Reason.REASON_MISS);
     }
   }
 
@@ -779,15 +776,13 @@ private static final class BackoffCacheEntry extends CacheEntry {
 
     private final Status status;
     private final BackoffPolicy backoffPolicy;
-    private final RouteLookupRequest.Reason routeLookupReason;
     private Future<?> scheduledFuture;
 
     BackoffCacheEntry(RouteLookupRequestKey routeLookupRequestKey, Status status,
-        BackoffPolicy backoffPolicy, RouteLookupRequest.Reason routeLookupReason) {
+        BackoffPolicy backoffPolicy) {
       super(routeLookupRequestKey);
       this.status = checkNotNull(status, "status");
       this.backoffPolicy = checkNotNull(backoffPolicy, "backoffPolicy");
-      this.routeLookupReason = checkNotNull(routeLookupReason, "routeLookupReason");
     }
 
     Status getStatus() {

rls/src/main/java/io/grpc/rls/RlsProtoData.java

@@ -47,9 +47,12 @@ abstract static class RouteLookupRequest {
 
     /** Names should match those in {@link io.grpc.lookup.v1.RouteLookupRequest.Reason} */
     enum Reason {
-      REASON_UNKNOWN,  // Unused
-      REASON_MISS,     // No data available in local cache
-      REASON_STALE;    // Data in local cache is stale
+      /** Unused */
+      REASON_UNKNOWN,
+      /** No data available in local cache */
+      REASON_MISS,
+      /** Data in local cache is stale */
+      REASON_STALE;
     }
 
     /** Reason for making this request. */

rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java

@@ -239,6 +239,7 @@ public void get_noError_lifeCycle() throws Exception {
     // cache hit for staled entry
     fakeClock.forwardTime(ROUTE_LOOKUP_CONFIG.staleAgeInNanos(), TimeUnit.NANOSECONDS);
 
+    rlsServerImpl.routeLookupReason = null;
     resp = getInSyncContext(routeLookupRequestKey);
     assertThat(resp.hasData()).isTrue();
 
@@ -250,6 +251,7 @@ public void get_noError_lifeCycle() throws Exception {
 
     resp = getInSyncContext(routeLookupRequestKey);
 
+    assertThat(rlsServerImpl.routeLookupReason).isEqualTo(io.grpc.lookup.v1.RouteLookupRequest.Reason.REASON_STALE);
     assertThat(resp.hasData()).isTrue();
 
     // existing cache expired
@@ -298,6 +300,7 @@ public void rls_withCustomRlsChannelServiceConfig() throws Exception {
             routeLookupRequestKey,
             RouteLookupResponse.create(ImmutableList.of("target"), "header")));
 
+    rlsServerImpl.routeLookupReason = null;
     // initial request
     CachedRouteLookupResponse resp = getInSyncContext(routeLookupRequestKey);
     assertThat(resp.isPending()).isTrue();
@@ -307,6 +310,7 @@ public void rls_withCustomRlsChannelServiceConfig() throws Exception {
 
     resp = getInSyncContext(routeLookupRequestKey);
     assertThat(resp.hasData()).isTrue();
+    assertThat(rlsServerImpl.routeLookupReason).isEqualTo(io.grpc.lookup.v1.RouteLookupRequest.Reason.REASON_MISS);
 
     assertThat(rlsChannelOverriddenAuthority).isEqualTo("bigtable.googleapis.com:443");
     assertThat(rlsChannelServiceConfig).isEqualTo(routeLookupChannelServiceConfig);
@@ -348,8 +352,10 @@ public void get_throttledAndRecover() throws Exception {
 
     assertThat(resp.isPending()).isTrue();
 
+    rlsServerImpl.routeLookupReason = null;
     // server responses
     fakeClock.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS);
+    assertThat(rlsServerImpl.routeLookupReason).isEqualTo(io.grpc.lookup.v1.RouteLookupRequest.Reason.REASON_MISS);
 
     resp = getInSyncContext(routeLookupRequestKey);
 
@@ -881,6 +887,7 @@ private static final class StaticFixedDelayRlsServerImpl
 
     private Map<RlsProtoData.RouteLookupRequestKey, RouteLookupResponse> lookupTable =
         ImmutableMap.of();
+    io.grpc.lookup.v1.RouteLookupRequest.Reason routeLookupReason;
 
     public StaticFixedDelayRlsServerImpl(
         long responseDelayNano, ScheduledExecutorService scheduledExecutorService) {
@@ -903,6 +910,7 @@ public void routeLookup(final io.grpc.lookup.v1.RouteLookupRequest request,
               new Runnable() {
                 @Override
                 public void run() {
+                  routeLookupReason = request.getReason();
                   RouteLookupResponse response =
                       lookupTable.get(
                           RlsProtoData.RouteLookupRequestKey.create(