Fix edge case where entities found for preview is empty #296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
============================================
+ Coverage 71.11% 71.29% +0.17%
+ Complexity 1873 1869 -4
============================================
Files 195 194 -1
Lines 9063 9018 -45
Branches 766 766
============================================
- Hits 6445 6429 -16
+ Misses 2251 2226 -25
+ Partials 367 363 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -72,6 +72,9 @@ public void executeDetector(AnomalyDetector detector, Instant startTime, Instant | |||
| if (categoryField != null && !categoryField.isEmpty()) { | |||
| featureManager.getPreviewEntities(detector, startTime.toEpochMilli(), endTime.toEpochMilli(), ActionListener.wrap(entities -> { | |||
|
|
|||
| if (entities == null || entities.isEmpty()) { | |||
| listener.onResponse(Collections.emptyList()); | |||
There was a problem hiding this comment.
Should we return empty results or some failure like this https://github.com/opendistro-for-elasticsearch/anomaly-detection/blob/master/src/main/java/com/amazon/opendistroforelasticsearch/ad/feature/FeatureManager.java#L525
There was a problem hiding this comment.
Such failure will be caught and empty result is still returned instead in onFailure()
I think it is okay to just return empty result
There was a problem hiding this comment.
Could you tell me why it would end up calling onFailure? listener is the rest channel.
There was a problem hiding this comment.
If entities is empty, code execution won't go into the for loop below, and listener has no chance to call onFailure or onResponse
There was a problem hiding this comment.
I feel it is better to return some failure and show that on kibana than return empty result. Customers may wonder why it returns nothing.
There was a problem hiding this comment.
if empty list is returned, existing Kibana will show corresponding message.
There was a problem hiding this comment.
Can you add a comment explaining why we want to return empty responses instead of errors? It helps code readability.
Issue #, if available:
Description of changes:
Fix edge case where entities found for preview is empty.
Have verified on test domain that empty result is returned with this fix.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.