From a42ee95f27de230fbe360fa4ab394b865fc13cb6 Mon Sep 17 00:00:00 2001 From: Kaituo Li Date: Mon, 9 Nov 2020 14:38:24 -0800 Subject: [PATCH 1/2] Fix for upgrading mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes two bugs of upgrading mapping: First, previously we only tried once when upgrading mapping.  It is possible we need to try a few more times.  The drawback is that if the code has a bug, we retry endlessly.  I should fix it in the future. Second, we enclose the upgrade mapping function call under a user context in a recent commit.  Upgrade mapping will fail when security plugin is enabled since a regular user cannot upgrade mappings of system indices. The PR also adds upper bounds to our dynamic settings if reasonable. Testing done: 1. Tested that upgrade mapping succeeds with new changes. 2. Tested the upper bounds of the changed settings are in effect. --- .../ad/AnomalyDetectorJobRunner.java | 3 ++- .../ad/indices/AnomalyDetectionIndices.java | 6 ++++- .../ad/settings/AnomalyDetectorSettings.java | 25 ++++++++++++++++--- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorJobRunner.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorJobRunner.java index 31345968..1a2f7f70 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorJobRunner.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorJobRunner.java @@ -210,6 +210,7 @@ protected void runAdJob( ); return; } + indexUtil.updateMappingIfNecessary(); /* * We need to handle 3 cases: * 1. Detectors created by older versions and never updated. These detectors wont have User details in the @@ -233,7 +234,7 @@ protected void runAdJob( try (InjectSecurity injectSecurity = new InjectSecurity(detectorId, settings, client.threadPool().getThreadContext())) { // Injecting user role to verify if the user has permissions for our API. injectSecurity.inject(user, roles); - indexUtil.updateMappingIfNecessary(); + AnomalyResultRequest request = new AnomalyResultRequest( detectorId, detectionStartTime.toEpochMilli(), diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java index 7e2fbcb4..0fbe2e6d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java @@ -578,7 +578,11 @@ public void updateMappingIfNecessary() { } final GroupedActionListener conglomerateListeneer = new GroupedActionListener<>( - ActionListener.wrap(r -> updateRunning.set(false), exception -> logger.error("Fail to updatea all mappings")), + ActionListener.wrap(r -> updateRunning.set(false), exception -> { + // TODO: don't retry endlessly. Can be annoying if there are too many exception logs. + updateRunning.set(false); + logger.error("Fail to updatea all mappings"); + }), updates.size() ); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/settings/AnomalyDetectorSettings.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/settings/AnomalyDetectorSettings.java index d9e17292..2c0e7fbd 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/settings/AnomalyDetectorSettings.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/settings/AnomalyDetectorSettings.java @@ -28,18 +28,27 @@ public final class AnomalyDetectorSettings { private AnomalyDetectorSettings() {} public static final Setting MAX_SINGLE_ENTITY_ANOMALY_DETECTORS = Setting - .intSetting("opendistro.anomaly_detection.max_anomaly_detectors", 1000, Setting.Property.NodeScope, Setting.Property.Dynamic); + .intSetting( + "opendistro.anomaly_detection.max_anomaly_detectors", + 1000, + 0, + 10_000, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); public static final Setting MAX_MULTI_ENTITY_ANOMALY_DETECTORS = Setting .intSetting( "opendistro.anomaly_detection.max_multi_entity_anomaly_detectors", 10, + 0, + 10_000, Setting.Property.NodeScope, Setting.Property.Dynamic ); public static final Setting MAX_ANOMALY_FEATURES = Setting - .intSetting("opendistro.anomaly_detection.max_anomaly_features", 5, Setting.Property.NodeScope, Setting.Property.Dynamic); + .intSetting("opendistro.anomaly_detection.max_anomaly_features", 5, 0, 100, Setting.Property.NodeScope, Setting.Property.Dynamic); public static final Setting REQUEST_TIMEOUT = Setting .positiveTimeSetting( @@ -251,7 +260,14 @@ private AnomalyDetectorSettings() {} // Increase the value will adding pressure to indexing anomaly results and our feature query public static final Setting MAX_ENTITIES_PER_QUERY = Setting - .intSetting("opendistro.anomaly_detection.max_entities_per_query", 1000, 1, Setting.Property.NodeScope, Setting.Property.Dynamic); + .intSetting( + "opendistro.anomaly_detection.max_entities_per_query", + 1000, + 1, + 100_000_000, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); // Default number of entities retrieved for Preview API public static final int DEFAULT_ENTITIES_FOR_PREVIEW = 30; @@ -262,6 +278,7 @@ private AnomalyDetectorSettings() {} "opendistro.anomaly_detection.max_entities_for_preview", DEFAULT_ENTITIES_FOR_PREVIEW, 1, + 1000, Setting.Property.NodeScope, Setting.Property.Dynamic ); @@ -278,7 +295,7 @@ private AnomalyDetectorSettings() {} // max number of primary shards of an AD index public static final Setting MAX_PRIMARY_SHARDS = Setting - .intSetting("opendistro.anomaly_detection.max_primary_shards", 10, 0, Setting.Property.NodeScope, Setting.Property.Dynamic); + .intSetting("opendistro.anomaly_detection.max_primary_shards", 10, 0, 200, Setting.Property.NodeScope, Setting.Property.Dynamic); // max entity value's length public static int MAX_ENTITY_LENGTH = 256; From ed6c3516a5bc1408e1fb5c509205afae323fc837 Mon Sep 17 00:00:00 2001 From: Kaituo Li Date: Mon, 9 Nov 2020 15:16:09 -0800 Subject: [PATCH 2/2] Update log message --- .../ad/indices/AnomalyDetectionIndices.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java index 0fbe2e6d..a398fcf0 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/indices/AnomalyDetectionIndices.java @@ -581,7 +581,7 @@ public void updateMappingIfNecessary() { ActionListener.wrap(r -> updateRunning.set(false), exception -> { // TODO: don't retry endlessly. Can be annoying if there are too many exception logs. updateRunning.set(false); - logger.error("Fail to updatea all mappings"); + logger.error("Fail to update AD indices' mappings"); }), updates.size() ); @@ -606,7 +606,14 @@ public void updateMappingIfNecessary() { } conglomerateListeneer.onResponse(null); }, exception -> { - logger.error(new ParameterizedMessage("Fail to update [{}]'s mapping", adIndex.getIndexName()), exception); + logger + .error( + new ParameterizedMessage( + "Fail to update [{}]'s mapping due to [{}]", + adIndex.getIndexName(), + exception.getMessage() + ) + ); conglomerateListeneer.onFailure(exception); }) );