[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #4271: Extend Detection Config Validator to validate composite(entity) alerts

[GitBox <gi...@apache.org>]

jihaozh commented on a change in pull request #4271: Extend Detection Config Validator to validate composite(entity) alerts 
URL: https://github.com/apache/incubator-pinot/pull/4271#discussion_r290413293
 
 

 ##########
 File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionConfigValidator.java
 ##########
 @@ -104,80 +112,155 @@ public void validateConfig(DetectionConfigDTO detectionConfig) throws IllegalArg
   /**
    * Validates the detection or filter rule accordingly based on {@param ruleType}
    */
-  private void validateRule(Map<String, Object> ruleYaml, int ruleIndex, String ruleType, Set<String> ruleNamesTaken) {
+  private void validateRule(String alertName, Map<String, Object> ruleYaml, int ruleIndex, String ruleType, Set<String> ruleNamesTaken) {
     Preconditions.checkArgument(ruleYaml.containsKey(PROP_TYPE),
-        "In rule no." + (ruleIndex) + ", " + ruleType + " rule type is missing.");
+        "Missing property " + ruleType + " (" + ruleType + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
     String type = MapUtils.getString(ruleYaml, PROP_TYPE);
-    String name = MapUtils.getString(ruleYaml, PROP_NAME);
-    Preconditions.checkNotNull(name,
-        "In rule no." + (ruleIndex) + ", " + ruleType + " rule name for type " +  type + " is missing.");
+
+    Preconditions.checkArgument(ruleYaml.containsKey(PROP_NAME),
+        "Missing property " + ruleType + " (" + PROP_NAME + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
+    String name = (String) ruleYaml.get(PROP_NAME);
+
     Preconditions.checkArgument(!ruleNamesTaken.contains(name),
-        "In rule No." + (ruleIndex) + ", found duplicate rule name, rule name must be unique within config." );
-    Preconditions.checkArgument(!name.contains(":"), "Sorry, rule name cannot contain \':\'");
+        "Duplicate rule name (" + name + ") found for sub-alert " + alertName + " rule no. " + ruleIndex + ". Names have to be unique within a config.");
+
+    Preconditions.checkArgument(!name.contains(":"),
+        "Illegal character (:) found in " + ruleType + " (" + PROP_NAME + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
   }
 
-  /**
-   * Validate the the detection yaml configuration.
-   *
-   * @param detectionYaml the detection yaml configuration to be validated
-   */
-  @Override
-  public void validateYaml(Map<String, Object> detectionYaml) {
+  private void validateBasicAttributes(Map<String, Object> detectionYaml, String parentAlertName) {
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_NAME),
+        "Missing property ( " + PROP_NAME + " ) in one of the sub-alerts under " + parentAlertName);
+    String alertName = (String) detectionYaml.get(PROP_NAME);
 
 Review comment:
   Can we use MapUtils.getString() instead of casting? That util function is more robust than typecasting.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org