You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/03/30 20:55:17 UTC
[GitHub] [incubator-pinot] jihaozh opened a new pull request #5196: [TE] fix
the merger issue that it can't merge historical anomaly generated by
multiple rules
jihaozh opened a new pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196
Issue: Child-keeping merge wrapper won't merge the historical anomaly generated by multiple rules.
Fix: Let the child-keeping merger retrieves the historical anomaly generated by multiple rules from the database and re-merges them. Previously this was ignored.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jihaozh merged pull request #5196: [TE] fix the
merger issue that it can't merge historical anomaly generated by multiple
rules
Posted by GitBox <gi...@apache.org>.
jihaozh merged pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r401204901
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/util/ThirdEyeUtils.java
##########
@@ -705,6 +706,16 @@ public static long getCachingPeriodLookback(TimeGranularity granularity) {
return period;
}
+ /**
+ * Check if the anomaly is detected by multiple components
+ * @param anomaly the anomaly
+ * @return if the anomaly is detected by multiple components
+ */
+ public static boolean isDetectedByMultipleComponents(MergedAnomalyResultDTO anomaly) {
+ String componentName = anomaly.getProperties().getOrDefault(PROP_DETECTOR_COMPONENT_NAME, "");
+ return componentName.contains(",");
Review comment:
Convert "," to a static final string PROP_DETECTOR_COMPONENT_NAME_DELIMETER and use this for all the places it is used. E.g, combineComponents()/
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r400561252
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
+ // retrieve the anomalies that are detected by multiple detectors
protected List<MergedAnomalyResultDTO> retrieveAnomaliesFromDatabase(List<MergedAnomalyResultDTO> generated) {
- return Collections.emptyList();
+ AnomalySlice effectiveSlice = this.slice.withDetectionId(this.config.getId())
+ .withStart(this.getStartTime(generated) - this.maxGap - 1)
+ .withEnd(this.getEndTime(generated) + this.maxGap + 1);
+
+ Collection<MergedAnomalyResultDTO> anomalies =
+ this.provider.fetchAnomalies(Collections.singleton(effectiveSlice)).get(effectiveSlice);
+
+ return anomalies.stream()
+ .filter(anomaly -> !anomaly.isChild() && isDetectedByMultipleComponents(anomaly))
Review comment:
Why we need to have a filter on multiple component here?
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r400560348
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
Review comment:
@jihaozh Do you know a reason why we had this logic?
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r400561152
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
+ // retrieve the anomalies that are detected by multiple detectors
protected List<MergedAnomalyResultDTO> retrieveAnomaliesFromDatabase(List<MergedAnomalyResultDTO> generated) {
- return Collections.emptyList();
+ AnomalySlice effectiveSlice = this.slice.withDetectionId(this.config.getId())
+ .withStart(this.getStartTime(generated) - this.maxGap - 1)
+ .withEnd(this.getEndTime(generated) + this.maxGap + 1);
+
+ Collection<MergedAnomalyResultDTO> anomalies =
+ this.provider.fetchAnomalies(Collections.singleton(effectiveSlice)).get(effectiveSlice);
+
+ return anomalies.stream()
+ .filter(anomaly -> !anomaly.isChild() && isDetectedByMultipleComponents(anomaly))
+ .collect(Collectors.toList());
+ }
+
+ private boolean isDetectedByMultipleComponents(MergedAnomalyResultDTO anomaly) {
+ String componentName = anomaly.getProperties().getOrDefault(PROP_DETECTOR_COMPONENT_NAME, "");
+ return componentName.contains(",");
Review comment:
This is too hacky. We can't depend on the string match to tell whether there are multiple components.
What if we decided to change the delimiter some time later then this is a bug hard to debug.
Let's put some helper function here to actually parse/assemble the component name in a centralized place.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
jihaozh commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r401199574
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
Review comment:
Previously, when two detectors detect the same anomaly duration, we retain the parent anomaly's detector name. So next time, when the pipeline runs, the parent anomaly will be retrieved from the rule-level merger. However, now we changed the parent anomaly's detector name to be the join of two detector names, the rule-level merger won't pick up the parent anomaly. The final merger needs to retrieve those anomalies and merges them.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
jihaozh commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r401223215
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/util/ThirdEyeUtils.java
##########
@@ -705,6 +706,16 @@ public static long getCachingPeriodLookback(TimeGranularity granularity) {
return period;
}
+ /**
+ * Check if the anomaly is detected by multiple components
+ * @param anomaly the anomaly
+ * @return if the anomaly is detected by multiple components
+ */
+ public static boolean isDetectedByMultipleComponents(MergedAnomalyResultDTO anomaly) {
+ String componentName = anomaly.getProperties().getOrDefault(PROP_DETECTOR_COMPONENT_NAME, "");
+ return componentName.contains(",");
Review comment:
good point. updated
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
jihaozh commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r401200455
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
+ // retrieve the anomalies that are detected by multiple detectors
protected List<MergedAnomalyResultDTO> retrieveAnomaliesFromDatabase(List<MergedAnomalyResultDTO> generated) {
- return Collections.emptyList();
+ AnomalySlice effectiveSlice = this.slice.withDetectionId(this.config.getId())
+ .withStart(this.getStartTime(generated) - this.maxGap - 1)
+ .withEnd(this.getEndTime(generated) + this.maxGap + 1);
+
+ Collection<MergedAnomalyResultDTO> anomalies =
+ this.provider.fetchAnomalies(Collections.singleton(effectiveSlice)).get(effectiveSlice);
+
+ return anomalies.stream()
+ .filter(anomaly -> !anomaly.isChild() && isDetectedByMultipleComponents(anomaly))
Review comment:
The rule-level merger will only fetch the anomalies generated by single rule. The final merger needs to retrieve the anomalies that are generated by multiple components and merges them.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request
#5196: [TE] fix the merger issue that it can't merge historical anomaly
generated by multiple rules
Posted by GitBox <gi...@apache.org>.
jihaozh commented on a change in pull request #5196: [TE] fix the merger issue that it can't merge historical anomaly generated by multiple rules
URL: https://github.com/apache/incubator-pinot/pull/5196#discussion_r401199718
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/ChildKeepingMergeWrapper.java
##########
@@ -48,9 +50,23 @@ public ChildKeepingMergeWrapper(DataProvider provider, DetectionConfigDTO config
}
@Override
- // does not fetch any anomalies from database
+ // retrieve the anomalies that are detected by multiple detectors
protected List<MergedAnomalyResultDTO> retrieveAnomaliesFromDatabase(List<MergedAnomalyResultDTO> generated) {
- return Collections.emptyList();
+ AnomalySlice effectiveSlice = this.slice.withDetectionId(this.config.getId())
+ .withStart(this.getStartTime(generated) - this.maxGap - 1)
+ .withEnd(this.getEndTime(generated) + this.maxGap + 1);
+
+ Collection<MergedAnomalyResultDTO> anomalies =
+ this.provider.fetchAnomalies(Collections.singleton(effectiveSlice)).get(effectiveSlice);
+
+ return anomalies.stream()
+ .filter(anomaly -> !anomaly.isChild() && isDetectedByMultipleComponents(anomaly))
+ .collect(Collectors.toList());
+ }
+
+ private boolean isDetectedByMultipleComponents(MergedAnomalyResultDTO anomaly) {
+ String componentName = anomaly.getProperties().getOrDefault(PROP_DETECTOR_COMPONENT_NAME, "");
+ return componentName.contains(",");
Review comment:
Updated.
----------------------------------------------------------------
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