From cafebc2a3d47deade8f21e2a8d901fcaa96b26eb Mon Sep 17 00:00:00 2001 From: Kaituo Li Date: Mon, 12 Oct 2020 12:11:17 -0700 Subject: [PATCH] Address comments --- .../ad/model/AnomalyDetector.java | 72 +++++++++---------- .../ad/model/AnomalyResult.java | 24 ++++--- .../rest/RestIndexAnomalyDetectorAction.java | 4 +- .../ADResultBulkTransportAction.java | 14 ++-- .../ad/model/AnomalyDetectorTests.java | 3 +- 5 files changed, 57 insertions(+), 60 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java index 3fcf1d14..094053a9 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java @@ -156,7 +156,7 @@ public AnomalyDetector( this.filterQuery = filterQuery; this.detectionInterval = detectionInterval; this.windowDelay = windowDelay; - this.shingleSize = shingleSize; + this.shingleSize = getShingleSize(shingleSize, categoryField); this.uiMetadata = uiMetadata; this.schemaVersion = schemaVersion; this.lastUpdateTime = lastUpdateTime; @@ -181,36 +181,23 @@ public AnomalyDetector( Integer schemaVersion, Instant lastUpdateTime ) { - if (Strings.isBlank(name)) { - throw new IllegalArgumentException("Detector name should be set"); - } - if (timeField == null) { - throw new IllegalArgumentException("Time field should be set"); - } - if (indices == null || indices.isEmpty()) { - throw new IllegalArgumentException("Indices should be set"); - } - if (detectionInterval == null) { - throw new IllegalArgumentException("Detection interval should be set"); - } - if (shingleSize != null && shingleSize < 1) { - throw new IllegalArgumentException("Shingle size must be a positive integer"); - } - this.detectorId = detectorId; - this.version = version; - this.name = name; - this.description = description; - this.timeField = timeField; - this.indices = indices; - this.featureAttributes = features; - this.filterQuery = filterQuery; - this.detectionInterval = detectionInterval; - this.windowDelay = windowDelay; - this.shingleSize = shingleSize; - this.uiMetadata = uiMetadata; - this.schemaVersion = schemaVersion; - this.lastUpdateTime = lastUpdateTime; - this.categoryField = null; + this( + detectorId, + version, + name, + description, + timeField, + indices, + features, + filterQuery, + detectionInterval, + windowDelay, + shingleSize, + uiMetadata, + schemaVersion, + lastUpdateTime, + null + ); } public XContentBuilder toXContent(XContentBuilder builder) throws IOException { @@ -272,7 +259,7 @@ public static AnomalyDetector parse(XContentParser parser, String detectorId) th * @throws IOException IOException if content can't be parsed correctly */ public static AnomalyDetector parse(XContentParser parser, String detectorId, Long version) throws IOException { - return parse(parser, detectorId, version, null, null, null); + return parse(parser, detectorId, version, null, null); } /** @@ -283,7 +270,6 @@ public static AnomalyDetector parse(XContentParser parser, String detectorId, Lo * @param version detector document version * @param defaultDetectionInterval default detection interval * @param defaultDetectionWindowDelay default detection window delay - * @param defaultShingleSize default number of intervals in shingle * @return anomaly detector instance * @throws IOException IOException if content can't be parsed correctly */ @@ -292,8 +278,7 @@ public static AnomalyDetector parse( String detectorId, Long version, TimeValue defaultDetectionInterval, - TimeValue defaultDetectionWindowDelay, - Integer defaultShingleSize + TimeValue defaultDetectionWindowDelay ) throws IOException { String name = null; String description = null; @@ -306,7 +291,7 @@ public static AnomalyDetector parse( TimeConfiguration windowDelay = defaultDetectionWindowDelay == null ? null : new IntervalTimeConfiguration(defaultDetectionWindowDelay.getSeconds(), ChronoUnit.SECONDS); - Integer shingleSize = defaultShingleSize; + Integer shingleSize = null; List features = new ArrayList<>(); int schemaVersion = 0; Map uiMetadata = null; @@ -497,9 +482,20 @@ public TimeConfiguration getWindowDelay() { } public Integer getShingleSize() { - return shingleSize == null + return shingleSize; + } + + /** + * If the given shingle size is null, return default based on the kind of detector; + * otherwise, return the given shingle size. + * @param customShingleSize Given shingle size + * @param categoryField Used to verify if this is a multi-entity or single-entity detector + * @return Shingle size + */ + private static Integer getShingleSize(Integer customShingleSize, List categoryField) { + return customShingleSize == null ? (categoryField != null && categoryField.size() > 0 ? DEFAULT_MULTI_ENTITY_SHINGLE : DEFAULT_SHINGLE_SIZE) - : shingleSize; + : customShingleSize; } public Map getUiMetadata() { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyResult.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyResult.java index fcaf8cad..78794acf 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyResult.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyResult.java @@ -85,17 +85,19 @@ public AnomalyResult( Instant executionEndTime, String error ) { - this.detectorId = detectorId; - this.anomalyScore = anomalyScore; - this.anomalyGrade = anomalyGrade; - this.confidence = confidence; - this.featureData = featureData; - this.dataStartTime = dataStartTime; - this.dataEndTime = dataEndTime; - this.executionStartTime = executionStartTime; - this.executionEndTime = executionEndTime; - this.error = error; - this.entity = null; + this( + detectorId, + anomalyScore, + anomalyGrade, + confidence, + featureData, + dataStartTime, + dataEndTime, + executionStartTime, + executionEndTime, + error, + null + ); } public AnomalyResult( diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/RestIndexAnomalyDetectorAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/RestIndexAnomalyDetectorAction.java index c713b20e..8629ec21 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/RestIndexAnomalyDetectorAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/RestIndexAnomalyDetectorAction.java @@ -15,7 +15,6 @@ package com.amazon.opendistroforelasticsearch.ad.rest; -import static com.amazon.opendistroforelasticsearch.ad.settings.AnomalyDetectorSettings.DEFAULT_SHINGLE_SIZE; import static com.amazon.opendistroforelasticsearch.ad.settings.AnomalyDetectorSettings.DETECTION_INTERVAL; import static com.amazon.opendistroforelasticsearch.ad.settings.AnomalyDetectorSettings.DETECTION_WINDOW_DELAY; import static com.amazon.opendistroforelasticsearch.ad.settings.AnomalyDetectorSettings.MAX_ANOMALY_DETECTORS; @@ -107,8 +106,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli XContentParser parser = request.contentParser(); ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); // TODO: check detection interval < modelTTL - AnomalyDetector detector = AnomalyDetector - .parse(parser, detectorId, null, detectionInterval, detectionWindowDelay, DEFAULT_SHINGLE_SIZE); + AnomalyDetector detector = AnomalyDetector.parse(parser, detectorId, null, detectionInterval, detectionWindowDelay); long seqNo = request.paramAsLong(IF_SEQ_NO, SequenceNumbers.UNASSIGNED_SEQ_NO); long primaryTerm = request.paramAsLong(IF_PRIMARY_TERM, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java index 7a7654b4..d2151711 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java @@ -107,12 +107,14 @@ protected void doExecute(Task task, ADResultBulkRequest request, ActionListener< } } - client - .execute( - BulkAction.INSTANCE, - bulkRequest, - ActionListener.wrap(response -> listener.onResponse(response), listener::onFailure) - ); + if (bulkRequest.numberOfActions() > 0) { + client + .execute( + BulkAction.INSTANCE, + bulkRequest, + ActionListener.wrap(response -> listener.onResponse(response), listener::onFailure) + ); + } } private void addResult(BulkRequest bulkRequest, AnomalyResult result) { diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetectorTests.java b/src/test/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetectorTests.java index da3ddcd9..2e20b3ec 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetectorTests.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetectorTests.java @@ -89,8 +89,7 @@ public void testParseAnomalyDetectorWithoutOptionalParams() throws IOException { + "{\"period\":{\"interval\":425,\"unit\":\"Minutes\"}},\"schema_version\":-1203962153,\"ui_metadata\":" + "{\"JbAaV\":{\"feature_id\":\"rIFjS\",\"feature_name\":\"QXCmS\",\"feature_enabled\":false," + "\"aggregation_query\":{\"aa\":{\"value_count\":{\"field\":\"ok\"}}}}},\"last_update_time\":1568396089028}"; - AnomalyDetector parsedDetector = AnomalyDetector - .parse(TestHelpers.parser(detectorString), "id", 1L, null, null, AnomalyDetectorSettings.DEFAULT_SHINGLE_SIZE); + AnomalyDetector parsedDetector = AnomalyDetector.parse(TestHelpers.parser(detectorString), "id", 1L, null, null); assertTrue(parsedDetector.getFilterQuery() instanceof MatchAllQueryBuilder); assertEquals((long) parsedDetector.getShingleSize(), (long) AnomalyDetectorSettings.DEFAULT_SHINGLE_SIZE); }