AMBARI-17863 : AMS - topN does not work when metric name has a wildcard specified. (avijayan)

ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/query/DefaultCondition.java

@@ -44,8 +44,8 @@ public class DefaultCondition implements Condition {
   private static final Log LOG = LogFactory.getLog(DefaultCondition.class);
 
   public DefaultCondition(List<String> metricNames, List<String> hostnames, String appId,
-                   String instanceId, Long startTime, Long endTime, Precision precision,
-                   Integer limit, boolean grouped) {
+                          String instanceId, Long startTime, Long endTime, Precision precision,
+                          Integer limit, boolean grouped) {
     this.metricNames = metricNames;
     this.hostnames = hostnames;
     this.appId = appId;
@@ -112,7 +112,7 @@ public class DefaultCondition implements Condition {
 
   public String getAppId() {
     if (appId != null && !appId.isEmpty()) {
-      if (!(appId.equals("HOST") || appId.equals("FLUME_HANDLER")) ) {
+      if (!(appId.equals("HOST") || appId.equals("FLUME_HANDLER"))) {
         return appId.toLowerCase();
       } else {
         return appId;
@@ -233,7 +233,7 @@ public class DefaultCondition implements Condition {
         }
       }
 
-      if (metricsIn.length()>0) {
+      if (metricsIn.length() > 0) {
         sb.append("(METRIC_NAME IN (");
         sb.append(metricsIn);
         sb.append(")");
@@ -313,4 +313,24 @@ public class DefaultCondition implements Condition {
       ", noLimit=" + noLimit +
       '}';
   }
+
+  protected static boolean metricNamesHaveWildcard(List<String> metricNames) {
+    for (String name : metricNames) {
+      if (name.contains("%")) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  protected static boolean hostNamesHaveWildcard(List<String> hostnames) {
+    if (hostnames == null)
+      return false;
+    for (String name : hostnames) {
+      if (name.contains("%")) {
+        return true;
+      }
+    }
+    return false;
+  }
 }

ambari-metrics/ambari-metrics-timelineservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/query/TopNCondition.java

@@ -150,7 +150,9 @@ public class TopNCondition extends DefaultCondition{
   public static boolean isTopNHostCondition(List<String> metricNames, List<String> hostnames) {
     // Case 1 : 1 Metric, H hosts
     // Select Top N or Bottom N host series based on 1 metric (max/avg/sum)
-    return (metricNames.size() == 1 && CollectionUtils.isNotEmpty(hostnames));
+    // Hostnames cannot be empty
+    // Only 1 metric allowed, without wildcards
+    return (CollectionUtils.isNotEmpty(hostnames) && metricNames.size() == 1 && !metricNamesHaveWildcard(metricNames));
 
   }
 
@@ -163,7 +165,10 @@ public class TopNCondition extends DefaultCondition{
   public static boolean isTopNMetricCondition(List<String> metricNames, List<String> hostnames) {
     // Case 2 : M Metric names or Regex, 1 or No host
     // Select Top N or Bottom N metric series based on metric values(max/avg/sum)
-    return (metricNames.size() > 1 && (hostnames == null || hostnames.size() <= 1));
+    // MetricNames cannot be empty
+    // No host (aggregate) or 1 host allowed, without wildcards
+    return (CollectionUtils.isNotEmpty(metricNames) && (hostnames == null || hostnames.size() <= 1) &&
+      !hostNamesHaveWildcard(hostnames));
   }
 
   public Integer getTopN() {

ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/TestPhoenixTransactSQL.java

@@ -620,31 +620,26 @@ public class TestPhoenixTransactSQL {
             "a1", "i1", 1407959718L, 1407959918L, null, null, false, 2, null, false);
 
     String conditionClause = condition.getConditionClause().toString();
-    String expectedClause = "(METRIC_NAME IN (?)) AND HOSTNAME IN (" +
-            "SELECT " + PhoenixTransactSQL.getNaiveTimeRangeHint(condition.getStartTime(),120000l) +
-            " HOSTNAME FROM METRIC_RECORD WHERE " +
-            "(METRIC_NAME IN (?)) AND " +
-            "(HOSTNAME LIKE ? OR HOSTNAME LIKE ?) AND " +
-            "APP_ID = ? AND INSTANCE_ID = ? AND " +
-            "SERVER_TIME >= ? AND SERVER_TIME < ? " +
-            "GROUP BY METRIC_NAME, HOSTNAME, APP_ID ORDER BY MAX(METRIC_MAX) DESC LIMIT 2) " +
-            "AND APP_ID = ? AND INSTANCE_ID = ? AND SERVER_TIME >= ? AND SERVER_TIME < ?";
+    String expectedClause = "(METRIC_NAME IN (?)) AND HOSTNAME IN (SELECT " +
+      PhoenixTransactSQL.getNaiveTimeRangeHint(condition.getStartTime(),120000l) +
+      " HOSTNAME FROM METRIC_RECORD WHERE (METRIC_NAME IN (?)) " +
+      "AND (HOSTNAME LIKE ? OR HOSTNAME LIKE ?) AND APP_ID = ? AND INSTANCE_ID = ? AND SERVER_TIME >= ? AND SERVER_TIME < ? GROUP BY " +
+      "METRIC_NAME, HOSTNAME, APP_ID ORDER BY MAX(METRIC_MAX) DESC LIMIT 2) AND APP_ID = ? AND INSTANCE_ID = ? AND " +
+      "SERVER_TIME >= ? AND SERVER_TIME < ?";
     Assert.assertEquals(expectedClause, conditionClause);
 
     condition = new TopNCondition(
-            Arrays.asList("m1", "m2", "m3"), Arrays.asList("%.ambari"),
+            Arrays.asList("m1"), Arrays.asList("%.ambari"),
             "a1", "i1", 1407959718L, 1407959918L, null, null, false, 2, null, false);
 
     conditionClause = condition.getConditionClause().toString();
-    expectedClause = " METRIC_NAME IN (" +
-            "SELECT " + PhoenixTransactSQL.getNaiveTimeRangeHint(condition.getStartTime(),120000l) +
-            " METRIC_NAME FROM METRIC_RECORD WHERE " +
-            "(METRIC_NAME IN (?, ?, ?)) AND " +
-            "(HOSTNAME LIKE ?) AND " +
-            "APP_ID = ? AND INSTANCE_ID = ? AND " +
-            "SERVER_TIME >= ? AND SERVER_TIME < ? " +
-            "GROUP BY METRIC_NAME, APP_ID ORDER BY MAX(METRIC_MAX) DESC LIMIT 2) " +
-            "AND (HOSTNAME LIKE ?) AND APP_ID = ? AND INSTANCE_ID = ? AND SERVER_TIME >= ? AND SERVER_TIME < ?";
+    expectedClause = "(METRIC_NAME IN (?)) AND HOSTNAME IN (SELECT " +
+      PhoenixTransactSQL.getNaiveTimeRangeHint(condition.getStartTime(),120000l) +
+      " HOSTNAME FROM METRIC_RECORD WHERE (METRIC_NAME IN (?)) " +
+      "AND (HOSTNAME LIKE ?) AND APP_ID = ? AND INSTANCE_ID = ? AND SERVER_TIME >= ? AND SERVER_TIME < ? GROUP BY " +
+      "METRIC_NAME, HOSTNAME, APP_ID ORDER BY MAX(METRIC_MAX) DESC LIMIT 2) AND APP_ID = ? AND INSTANCE_ID = ? AND " +
+      "SERVER_TIME >= ? AND SERVER_TIME < ?";
+
     Assert.assertEquals(expectedClause, conditionClause);
 
     condition = new TopNCondition(

ambari-metrics/ambari-metrics-timelineservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/metrics/timeline/TopNConditionTest.java

@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.applicationhistoryservice.metrics.timeline;
+
+import junit.framework.Assert;
+import org.apache.hadoop.yarn.server.applicationhistoryservice.metrics.timeline.query.TopNCondition;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class TopNConditionTest {
+
+
+  @Test
+  public void testTopNCondition() {
+
+    List<String> metricNames = new ArrayList<>();
+    List<String> hostnames = new ArrayList<>();
+
+    //Valid Cases
+
+    // "H" hosts and 1 Metric
+    hostnames.add("h1");
+    hostnames.add("h2");
+    metricNames.add("m1");
+    Assert.assertTrue(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertFalse(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+    hostnames.clear();
+
+    // Host(s) with wildcard & 1 Metric
+    hostnames.add("h%");
+    hostnames.add("g1");
+    Assert.assertTrue(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertFalse(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+    hostnames.clear();
+
+    // M Metrics and No host
+    metricNames.add("m2");
+    metricNames.add("m3");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertTrue(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+    // M Metrics with wildcard and No host
+    metricNames.add("m2");
+    metricNames.add("m%");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertTrue(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+    // M Metrics with wildcard and 1 host
+    metricNames.add("m2");
+    metricNames.add("m%");
+    hostnames.add("h1");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertTrue(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+    metricNames.clear();
+    hostnames.clear();
+
+    //Invalid Cases
+    // M metrics and H hosts
+    metricNames.add("m1");
+    metricNames.add("m2");
+    hostnames.add("h1");
+    hostnames.add("h2");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertFalse(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+    metricNames.clear();
+    hostnames.clear();
+
+    // Wildcard in 1 and multiple in other
+    metricNames.add("m1");
+    metricNames.add("m2");
+    hostnames.add("%");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertFalse(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+    metricNames.clear();
+    hostnames.clear();
+
+    //Wildcard in both
+    metricNames.add("m%");
+    hostnames.add("%");
+    Assert.assertFalse(TopNCondition.isTopNHostCondition(metricNames, hostnames));
+    Assert.assertFalse(TopNCondition.isTopNMetricCondition(metricNames, hostnames));
+
+  }
+}