Cleanup and added some more informative comments

zhilingc · zhilingc · commit 0fca98e2bf90 · 2019-10-07T11:14:07.000+08:00
diff --git a/protos/feast/core/CoreService.proto b/protos/feast/core/CoreService.proto
@@ -30,25 +30,47 @@ service CoreService {
     rpc GetFeastCoreVersion (GetFeastCoreVersionRequest) returns (GetFeastCoreVersionResponse);
 
     // Retrieve feature set details given a filter.
-    // Returns all featureSets matching that filter.
+    //
+    // Returns all feature sets matching that filter. If none are found,
+    // an empty list will be returned.
+    // If no filter is provided in the request, the response will contain all the feature
+    // sets currently stored in the registry.
     rpc GetFeatureSets (GetFeatureSetsRequest) returns (GetFeatureSetsResponse);
 
     // Retrieve store details given a filter.
-    // Returns all stores matching that filter.
+    //
+    // Returns all stores matching that filter. If none are found, an empty list will be returned.
+    // If no filter is provided in the request, the response will contain all the stores currently
+    // stored in the registry.
     rpc GetStores (GetStoresRequest) returns (GetStoresResponse);
 
-    // Idempotent creation of feature set. Will not create a new feature set if schema does not change.
+    // Create or update and existing feature set.
+    //
+    // This function is idempotent - it will not create a new feature set if schema does not change.
+    // If an existing feature set is updated, core will advance the version number, which will be
+    // returned in response.
     rpc ApplyFeatureSet (ApplyFeatureSetRequest) returns (ApplyFeatureSetResponse);
 
-    // Updates core with the configuration of the store. If the changes are valid,
-    // core will return the given store configuration in response.
+    // Updates core with the configuration of the store.
+    //
+    // If the changes are valid, core will return the given store configuration in response, and
+    // start or update the necessary feature population jobs for the updated store.
     rpc UpdateStore(UpdateStoreRequest) returns (UpdateStoreResponse);
 }
 
 // Retrieves details for all versions of a specific feature set
 message GetFeatureSetsRequest {
     message Filter {
+        // Name of the desired feature set. Valid regex strings are allowed.
+        // e.g.
+        // - .* can be used to match all feature sets
+        // - my-project-.* can be used to match all features prefixed by "my-project"
         string feature_set_name = 1;
+        // Version of the desired feature set. Either a number or valid expression can be provided.
+        // e.g.
+        // - 1 will match version 1 exactly
+        // - >=1 will match all versions greater or equal to 1
+        // - <10 will match all versions less than 10
         string feature_set_version = 2;
     }
 
@@ -61,6 +83,7 @@ message GetFeatureSetsResponse {
 
 message GetStoresRequest {
     message Filter {
+        // Name of desired store. Regex is not supported in this query.
         string name = 1;
     }
 
diff --git a/serving/sample_redis_config.yml b/serving/sample_redis_config.yml
@@ -4,5 +4,5 @@ redis_config:
   host: localhost
   port: 6379
 subscriptions:
-- name: .*
-  version: ">0"
+  - name: .*
+    version: ">0"
diff --git a/serving/src/main/java/feast/serving/controller/HealthServiceController.java b/serving/src/main/java/feast/serving/controller/HealthServiceController.java
@@ -1,5 +1,6 @@
 package feast.serving.controller;
 
+import feast.core.StoreProto.Store;
 import feast.serving.ServingAPIProto.GetFeastServingTypeRequest;
 import feast.serving.service.CachedSpecService;
 import feast.serving.service.ServingService;
@@ -32,7 +33,7 @@ public void check(
     //       Implement similary for batch service.
 
     try {
-      specService.getStore();
+      Store store = specService.getStore();
       servingService.getFeastServingType(GetFeastServingTypeRequest.getDefaultInstance());
       responseObserver.onNext(
           HealthCheckResponse.newBuilder().setStatus(ServingStatus.SERVING).build());
diff --git a/serving/src/main/java/feast/serving/service/CachedSpecService.java b/serving/src/main/java/feast/serving/service/CachedSpecService.java
@@ -25,10 +25,12 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
+import lombok.extern.slf4j.Slf4j;
 
 /**
- * SpecStorage implementation with built-in in-memory cache.
+ * In-memory cache of specs.
  */
+@Slf4j
 public class CachedSpecService {
 
   private static final int MAX_SPEC_COUNT = 1000;
@@ -53,16 +55,28 @@ public CachedSpecService(CoreSpecService coreService, Path configPath) {
         CacheBuilder.newBuilder().maximumSize(MAX_SPEC_COUNT).build(featureSetSpecCacheLoader);
   }
 
+  /**
+   * Get the current store configuration.
+   *
+   * @return StoreProto.Store store configuration for this serving instance
+   */
   public Store getStore() {
     return this.store;
   }
 
+  /**
+   * Get a single FeatureSetSpec matching the given name and version.
+   *
+   * @param name of the featureSet
+   * @param version to retrieve
+   * @return FeatureSetSpec of the matching FeatureSet
+   */
   public FeatureSetSpec getFeatureSet(String name, int version) {
     String id = String.format("%s:%d", name, version);
     try {
       return featureSetSpecCache.get(id);
     } catch (InvalidCacheLoadException e) {
-      // if not found, default to core
+      // if not found, try to retrieve from core
       GetFeatureSetsRequest request = GetFeatureSetsRequest.newBuilder()
           .setFilter(Filter.newBuilder()
               .setFeatureSetName(name)
@@ -83,7 +97,8 @@ public FeatureSetSpec getFeatureSet(String name, int version) {
   }
 
   /**
-   * Preload all specs into the cache.
+   * Reload the store configuration from the given config path, then retrieve the necessary specs
+   * from core to preload the cache.
    */
   public void populateCache() {
     this.store = updateStore(readConfig(configPath));
@@ -110,7 +125,8 @@ private Map<String, FeatureSetSpec> getFeatureSetSpecMap() {
                   featureSetSpec);
         }
       } catch (StatusRuntimeException e) {
-        continue;
+        throw new RuntimeException(
+            String.format("Unable to retrieve specs matching subscription %s", subscription), e);
       }
     }
     return featureSetSpecs;
diff --git a/serving/src/main/java/feast/serving/service/CoreSpecService.java b/serving/src/main/java/feast/serving/service/CoreSpecService.java
@@ -9,10 +9,9 @@
 import io.grpc.ManagedChannelBuilder;
 import lombok.extern.slf4j.Slf4j;
 
-// TODO: Health check and recovery for this CoreSpecService, i.e.
-//       if client fails to connect to Feast Core GRPC service, what to do or report?
-//       By default, managed channel should do auto-retry etc, but just to double check.
-
+/**
+ * Client for spec retrieval from core.
+ */
 @Slf4j
 public class CoreSpecService {
   private final CoreServiceGrpc.CoreServiceBlockingStub blockingStub;
diff --git a/serving/src/main/resources/application.yml b/serving/src/main/resources/application.yml
@@ -19,7 +19,7 @@ feast:
 
   store:
     # Path containing the store configuration for this serving store.
-    config-path: ${FEAST_STORE_CONFIG_PATH:/etc/feast/sample_redis_config.yml}
+    config-path: ${FEAST_STORE_CONFIG_PATH:./sample_redis_config.yml}
     # If serving redis, the redis pool max size
     redis-pool-max-size: 128
     # If serving redis, the redis pool max idle conns
