Addressed review comments.

o19s · Aug 27, 2020 · 00a4b96 · 00a4b96
1 parent c3520bb
commit 00a4b96
Show file tree

Hide file tree

Showing 18 changed files with 199 additions and 206 deletions.
diff --git a/docs/advanced-functionality.rst b/docs/advanced-functionality.rst
@@ -218,7 +218,7 @@ Stats
 =============================
 The stats API gives the overall plugin status and statistics::
 
-    GET /_ltr/stats
+    GET /_ltr/_stats
 
     {
         "_nodes": {
@@ -267,10 +267,10 @@ The stats API gives the overall plugin status and statistics::
 
 You can also use filters to retrieve a single stat::
 
-    GET /_ltr/stats/{stat}
+    GET /_ltr/_stats/{stat}
 
 Also you can limit the information to a single node in the cluster::
 
-    GET /_ltr/stats/nodes/{nodeId}
+    GET /_ltr/_stats/nodes/{nodeId}
 
-    GET /_ltr/stats/{stat}/nodes/{nodeId}
+    GET /_ltr/_stats/{stat}/nodes/{nodeId}
diff --git a/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java b/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java
@@ -49,7 +49,6 @@
 import com.o19s.es.ltr.ranker.parser.XGBoostJsonParser;
 import com.o19s.es.ltr.ranker.ranklib.RankLibScriptEngine;
 import com.o19s.es.ltr.ranker.ranklib.RanklibModelParser;
-import com.o19s.es.ltr.rest.RestAddFeatureToSet;
 import com.o19s.es.ltr.rest.RestCreateModelFromSet;
 import com.o19s.es.ltr.rest.RestFeatureManager;
 import com.o19s.es.ltr.rest.RestSearchStoreElements;
@@ -259,13 +258,13 @@ public Collection<Object> createComponents(Client client,
     }
 
     private LTRStats getStats(Client client, ClusterService clusterService) {
-        Map<String, LTRStat<?>> stats = new HashMap<>();
+        Map<String, LTRStat> stats = new HashMap<>();
         stats.put(StatName.CACHE.getName(),
-                new LTRStat<>(false, new CacheStatsOnNodeSupplier(caches)));
+                new LTRStat(false, new CacheStatsOnNodeSupplier(caches)));
         stats.put(StatName.STORES.getName(),
-                new LTRStat<>(true, new StoreStatsSupplier(client, clusterService)));
+                new LTRStat(true, new StoreStatsSupplier(client, clusterService)));
         stats.put(StatName.PLUGIN_STATUS.getName(),
-                new LTRStat<>(true, new PluginHealthStatusSupplier(clusterService)));
+                new LTRStat(true, new PluginHealthStatusSupplier(clusterService)));
         return new LTRStats(unmodifiableMap(stats));
     }
 

diff --git a/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java b/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java
@@ -5,14 +5,12 @@
 import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsNodesRequest;
 import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsNodesResponse;
 import com.o19s.es.ltr.stats.LTRStats;
-import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.FailedNodeException;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.nodes.TransportNodesAction;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.tasks.Task;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.transport.TransportService;
 
@@ -39,11 +37,6 @@ public TransportLTRStatsAction(ThreadPool threadPool,
         this.ltrStats = ltrStats;
     }
 
-    @Override
-    protected void doExecute(Task task, LTRStatsNodesRequest request, ActionListener<LTRStatsNodesResponse> listener) {
-        super.doExecute(task, request, listener);
-    }
-
     @Override
     protected LTRStatsNodesResponse newResponse(LTRStatsNodesRequest request,
                                                 List<LTRStatsNodeResponse> nodeResponses,
@@ -54,7 +47,7 @@ protected LTRStatsNodesResponse newResponse(LTRStatsNodesRequest request,
                         .entrySet()
                         .stream()
                         .filter(e -> statsToBeRetrieved.contains(e.getKey()))
-                        .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue()));
+                        .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getStatValue()));
 
         return new LTRStatsNodesResponse(clusterService.getClusterName(), nodeResponses, failures, clusterStats);
     }
@@ -79,7 +72,7 @@ protected LTRStatsNodeResponse nodeOperation(LTRStatsNodeRequest request) {
                         .entrySet()
                         .stream()
                         .filter(e -> statsToBeRetrieved.contains(e.getKey()))
-                        .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue()));
+                        .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getStatValue()));
         return new LTRStatsNodeResponse(clusterService.localNode(), statValues);
     }
 }
diff --git a/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java b/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java
@@ -25,7 +25,7 @@
  * APIs to retrieve stats on the plugin usage and performance.
  */
 public class RestLTRStats extends BaseRestHandler {
-    public static final String LTR_STATS_BASE_URI = "/_ltr/stats";
+    public static final String LTR_STATS_BASE_URI = "/_ltr/_stats";
     private static final String NAME = "learning_to_rank_stats";
 
     @Override
@@ -76,7 +76,7 @@ private Set<String> getStatsToBeRetrieved(
             RestRequest request, Set<String> validStats, List<String> requestedStats) {
         if (requestedStats.contains(LTRStatsNodesRequest.ALL_STATS_KEY)) {
             throw new IllegalArgumentException(
-                    String.format(Locale.getDefault(), "Request %s contains both %s and individual stats",
+                    String.format(Locale.ROOT, "Request %s contains both %s and individual stats",
                             request.path(), LTRStatsNodesRequest.ALL_STATS_KEY));
         }
 

diff --git a/src/main/java/com/o19s/es/ltr/stats/LTRStat.java b/src/main/java/com/o19s/es/ltr/stats/LTRStat.java
@@ -1,22 +1,17 @@
 package com.o19s.es.ltr.stats;
 
-import com.o19s.es.ltr.stats.suppliers.CounterSupplier;
-
 import java.util.function.Supplier;
 
 /**
  * A container for a stat provided by the plugin. Each instance is associated with
- * an underlying supplier. The supplier can be a counter in which case it can be
- * incremented. The stat instance also stores a flag to indicate whether this is
- * a cluster level or node level stat.
- *
- * @param <T> the type of the value returned by the stat.
+ * an underlying supplier. The stat instance also stores a flag to indicate whether
+ * this is a cluster level or a node level stat.
  */
-public class LTRStat<T> {
+public class LTRStat {
     private final boolean clusterLevel;
-    private final Supplier<T> supplier;
+    private final Supplier<?> supplier;
 
-    public LTRStat(boolean clusterLevel, Supplier<T> supplier) {
+    public LTRStat(boolean clusterLevel, Supplier<?> supplier) {
         this.clusterLevel = clusterLevel;
         this.supplier = supplier;
     }
@@ -25,15 +20,7 @@ public boolean isClusterLevel() {
         return clusterLevel;
     }
 
-    public T getValue() {
+    public Object getStatValue() {
         return supplier.get();
     }
-
-    public void increment() {
-        if (!(supplier instanceof CounterSupplier)) {
-            throw new UnsupportedOperationException(
-                    "cannot increment the supplier: " + supplier.getClass().getName());
-        }
-        ((CounterSupplier) supplier).increment();
-    }
 }
diff --git a/src/main/java/com/o19s/es/ltr/stats/LTRStats.java b/src/main/java/com/o19s/es/ltr/stats/LTRStats.java
@@ -8,34 +8,34 @@
  * This class is the main entry-point for access to the stats that the LTR plugin keeps track of.
  */
 public class LTRStats {
-    private final Map<String, LTRStat<?>> stats;
+    private final Map<String, LTRStat> stats;
 
 
-    public LTRStats(Map<String, LTRStat<?>> stats) {
+    public LTRStats(Map<String, LTRStat> stats) {
         this.stats = stats;
     }
 
-    public Map<String, LTRStat<?>> getStats() {
+    public Map<String, LTRStat> getStats() {
         return stats;
     }
 
-    public LTRStat<?> getStat(String key) throws IllegalArgumentException {
-        LTRStat<?> stat = stats.get(key);
+    public LTRStat getStat(String key) throws IllegalArgumentException {
+        LTRStat stat = stats.get(key);
         if (stat == null) {
             throw new IllegalArgumentException("Stat=\"" + key + "\" does not exist");
         }
         return stat;
     }
 
-    public Map<String, LTRStat<?>> getNodeStats() {
+    public Map<String, LTRStat> getNodeStats() {
         return getClusterOrNodeStats(false);
     }
 
-    public Map<String, LTRStat<?>> getClusterStats() {
+    public Map<String, LTRStat> getClusterStats() {
         return getClusterOrNodeStats(true);
     }
 
-    private Map<String, LTRStat<?>> getClusterOrNodeStats(Boolean isClusterStats) {
+    private Map<String, LTRStat> getClusterOrNodeStats(Boolean isClusterStats) {
         return stats.entrySet()
                 .stream()
                 .filter(e -> e.getValue().isClusterLevel() == isClusterStats)

diff --git a/src/main/java/com/o19s/es/ltr/stats/StatName.java b/src/main/java/com/o19s/es/ltr/stats/StatName.java
@@ -7,22 +7,7 @@
 public enum StatName {
     PLUGIN_STATUS("status"),
     STORES("stores"),
-    CACHE("cache"),
-
-    STORE_STATUS("status"),
-    STORE_FEATURE_COUNT("feature_count"),
-    STORE_FEATURE_SET_COUNT("featureset_count"),
-    STORE_MODEL_COUNT("model_count"),
-
-    CACHE_FEATURE("feature"),
-    CACHE_FEATURE_SET("featureset"),
-    CACHE_MODEL("model"),
-
-    CACHE_HIT_COUNT("hit_count"),
-    CACHE_MISS_COUNT("miss_count"),
-    CACHE_EVICTION_COUNT("eviction_count"),
-    CACHE_ENTRY_COUNT("entry_count"),
-    CACHE_MEMORY_USAGE_IN_BYTES("memory_usage_in_bytes");
+    CACHE("cache");
 
     private final String name;
 

diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java
@@ -1,7 +1,6 @@
 package com.o19s.es.ltr.stats.suppliers;
 
 import com.o19s.es.ltr.feature.store.index.Caches;
-import com.o19s.es.ltr.stats.StatName;
 import org.elasticsearch.common.cache.Cache;
 
 import java.util.Collections;
@@ -15,26 +14,48 @@
 public class CacheStatsOnNodeSupplier implements Supplier<Map<String, Map<String, Object>>> {
     private final Caches caches;
 
+    public enum Stat {
+        CACHE_FEATURE("feature"),
+        CACHE_FEATURE_SET("featureset"),
+        CACHE_MODEL("model"),
+
+        CACHE_HIT_COUNT("hit_count"),
+        CACHE_MISS_COUNT("miss_count"),
+        CACHE_EVICTION_COUNT("eviction_count"),
+        CACHE_ENTRY_COUNT("entry_count"),
+        CACHE_MEMORY_USAGE_IN_BYTES("memory_usage_in_bytes");
+
+        private final String name;
+
+        Stat(String name) {
+            this.name = name;
+        }
+
+        public String getName() {
+            return name;
+        }
+    }
+
     public CacheStatsOnNodeSupplier(Caches caches) {
         this.caches = caches;
     }
 
     @Override
     public Map<String, Map<String, Object>> get() {
         Map<String, Map<String, Object>> values = new HashMap<>();
-        values.put(StatName.CACHE_FEATURE.getName(), getCacheStats(caches.featureCache()));
-        values.put(StatName.CACHE_FEATURE_SET.getName(), getCacheStats(caches.featureSetCache()));
-        values.put(StatName.CACHE_MODEL.getName(), getCacheStats(caches.modelCache()));
+        values.put(Stat.CACHE_FEATURE.getName(), getCacheStats(caches.featureCache()));
+        values.put(Stat.CACHE_FEATURE_SET.getName(), getCacheStats(caches.featureSetCache()));
+        values.put(Stat.CACHE_MODEL.getName(), getCacheStats(caches.modelCache()));
         return Collections.unmodifiableMap(values);
     }
 
     private Map<String, Object> getCacheStats(Cache<Caches.CacheKey, ?> cache) {
         Map<String, Object> stat = new HashMap<>();
-        stat.put(StatName.CACHE_HIT_COUNT.getName(), cache.stats().getHits());
-        stat.put(StatName.CACHE_MISS_COUNT.getName(), cache.stats().getMisses());
-        stat.put(StatName.CACHE_EVICTION_COUNT.getName(), cache.stats().getEvictions());
-        stat.put(StatName.CACHE_ENTRY_COUNT.getName(), cache.count());
-        stat.put(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName(), cache.weight());
+        stat.put(Stat.CACHE_HIT_COUNT.getName(), cache.stats().getHits());
+        stat.put(Stat.CACHE_MISS_COUNT.getName(), cache.stats().getMisses());
+        stat.put(Stat.CACHE_EVICTION_COUNT.getName(), cache.stats().getEvictions());
+        stat.put(Stat.CACHE_ENTRY_COUNT.getName(), cache.count());
+        stat.put(Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName(), cache.weight());
         return Collections.unmodifiableMap(stat);
     }
 }
diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java
diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java
@@ -45,7 +45,7 @@ private String getAggregateStoresStatus() {
     }
 
     private String combineStatuses(String s1, String s2) {
-        if (s2 == null || STATUS_RED.equals(s1) || STATUS_RED.equals(s2)) {
+        if (STATUS_RED.equals(s1) || STATUS_RED.equals(s2)) {
             return STATUS_RED;
         } else if (STATUS_YELLOW.equals(s1) || STATUS_YELLOW.equals(s2)) {
             return STATUS_YELLOW;
@@ -60,6 +60,6 @@ public String getLtrStoreHealthStatus(String storeName) {
                 clusterService.state().getRoutingTable().index(storeName)
         );
 
-        return indexHealth.getStatus().name().toLowerCase(Locale.getDefault());
+        return indexHealth.getStatus().name().toLowerCase(Locale.ROOT);
     }
 }