From e2a2fb931d35a3fc9e7fcd0b20b1d528b269177c Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Tue, 21 Sep 2021 12:46:57 +0200 Subject: [PATCH 1/6] Speed up DataTierAllocationDecider This decider is quite slow in `allocationAllowed` and shows up in profiling. We should be able to get a much tighter loop with this change that avoids building the list of role names over and over and removes some dead code as well. --- .../allocation/DataTierAllocationDecider.java | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index f9c5fdcf01d53..57168b71551b1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -28,6 +28,7 @@ import java.util.Arrays; import java.util.Collection; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -160,7 +161,7 @@ private Decision shouldIndexPreferTier(IndexMetadata indexMetadata, Set roles) { + private static boolean allocationAllowed(String tierSetting, Set roles) { String[] values = parseTierList(tierSetting); - for (String value : values) { + Set roleNames = null; + if (values.length == 0) { + return true; + } + if (roles.contains(DiscoveryNodeRole.DATA_ROLE)) { // generic "data" roles are considered to have all tiers - if (roles.contains(DiscoveryNodeRole.DATA_ROLE) || - roles.stream().map(DiscoveryNodeRole::roleName).collect(Collectors.toSet()).contains(value)) { - if (opType == OpType.OR) { + return true; + } + if (values.length == 1) { + final String value = values[0]; + for (DiscoveryNodeRole role : roles) { + if (value.equals(role.roleName())) { return true; } - } else { - if (opType == OpType.AND) { - return false; - } } - } - if (opType == OpType.OR) { - return false; } else { - return true; + for (String value : values) { + if (roleNames == null) { + roleNames = new HashSet<>(roles.size()); + for (DiscoveryNodeRole role : roles) { + roleNames.add(role.roleName()); + } + if (roleNames.contains(value) == false) { + return false; + } + } + } } + return true; } } From 29ce6c8b7d910d0b1af604e25e5246e6681a270e Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Tue, 21 Sep 2021 13:14:29 +0200 Subject: [PATCH 2/6] logic is hard --- .../routing/allocation/DataTierAllocationDecider.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index 57168b71551b1..2829323d338b0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -222,6 +222,7 @@ private static boolean allocationAllowed(String tierSetting, Set Date: Tue, 21 Sep 2021 21:27:34 +0200 Subject: [PATCH 3/6] incorporate review --- .../allocation/DataTierAllocationDecider.java | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index 2829323d338b0..ef354ad3dd10b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -188,7 +188,12 @@ public static Optional preferredAvailableTier(String prioritizedTiers, D } public static String[] parseTierList(String tiers) { - return Strings.tokenizeToStringArray(tiers, ","); + if (Strings.hasLength(tiers) == false) { + // avoid parsing overhead in the null/empty string case + return Strings.EMPTY_ARRAY; + } else { + return Strings.tokenizeToStringArray(tiers, ","); + } } static boolean tierNodesPresent(String singleTier, DiscoveryNodes nodes) { @@ -207,7 +212,6 @@ static boolean tierNodesPresent(String singleTier, DiscoveryNodes nodes) { private static boolean allocationAllowed(String tierSetting, Set roles) { String[] values = parseTierList(tierSetting); - Set roleNames = null; if (values.length == 0) { return true; } @@ -224,18 +228,11 @@ private static boolean allocationAllowed(String tierSetting, Set(roles.size()); - for (DiscoveryNodeRole role : roles) { - roleNames.add(role.roleName()); - } - } - if (roleNames.contains(value) == false) { - return false; - } + final Set roleNames = new HashSet<>(roles.size()); + for (DiscoveryNodeRole role : roles) { + roleNames.add(role.roleName()); } + return roleNames.containsAll(Arrays.asList(values)); } - return true; } } From 9ca47b0c44989ab7f445238a99719452aeadb631 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 21 Sep 2021 22:45:02 -0400 Subject: [PATCH 4/6] Remove outdated comment --- .../cluster/routing/allocation/DataTierAllocationDecider.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index ef354ad3dd10b..4a5a4092e442e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -159,8 +159,6 @@ private Decision shouldIndexPreferTier(IndexMetadata indexMetadata, Set tier = preferredTierFunction.apply(tierPreference, allocation.nodes()); if (tier.isPresent()) { String tierName = tier.get(); - // The OpType doesn't actually matter here, because we have - // selected only a single tier as our "preferred" tier if (allocationAllowed(tierName, roles)) { return allocation.decision(Decision.YES, NAME, "index has a preference for tiers [%s] and node has tier [%s]", tierPreference, tierName); From d4668a8c17c1e4499a722f67b7c01b3af77c9022 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 21 Sep 2021 22:45:19 -0400 Subject: [PATCH 5/6] Internal refactor The tierName argument is a single tier name, rather than comma separated list of zero or more tier names. --- .../allocation/DataTierAllocationDecider.java | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index 4a5a4092e442e..7537d73ab755c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -28,7 +28,6 @@ import java.util.Arrays; import java.util.Collection; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -208,29 +207,19 @@ static boolean tierNodesPresent(String singleTier, DiscoveryNodes nodes) { } - private static boolean allocationAllowed(String tierSetting, Set roles) { - String[] values = parseTierList(tierSetting); - if (values.length == 0) { - return true; - } + private static boolean allocationAllowed(String tierName, Set roles) { + assert Strings.hasLength(tierName) : "tierName must be not null and non-empty, but was [" + tierName + "]"; + if (roles.contains(DiscoveryNodeRole.DATA_ROLE)) { // generic "data" roles are considered to have all tiers return true; - } - if (values.length == 1) { - final String value = values[0]; + } else { for (DiscoveryNodeRole role : roles) { - if (value.equals(role.roleName())) { + if (tierName.equals(role.roleName())) { return true; } } return false; - } else { - final Set roleNames = new HashSet<>(roles.size()); - for (DiscoveryNodeRole role : roles) { - roleNames.add(role.roleName()); - } - return roleNames.containsAll(Arrays.asList(values)); } } } From 80c72211e7d694fe57ddae2ff81395d5daa177ec Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Wed, 22 Sep 2021 09:32:56 +0200 Subject: [PATCH 6/6] CR: comments --- .../cluster/routing/allocation/DataTierAllocationDecider.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java index 7537d73ab755c..c0119aa7feb95 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/DataTierAllocationDecider.java @@ -185,7 +185,7 @@ public static Optional preferredAvailableTier(String prioritizedTiers, D } public static String[] parseTierList(String tiers) { - if (Strings.hasLength(tiers) == false) { + if (Strings.hasText(tiers) == false) { // avoid parsing overhead in the null/empty string case return Strings.EMPTY_ARRAY; } else { @@ -208,7 +208,7 @@ static boolean tierNodesPresent(String singleTier, DiscoveryNodes nodes) { private static boolean allocationAllowed(String tierName, Set roles) { - assert Strings.hasLength(tierName) : "tierName must be not null and non-empty, but was [" + tierName + "]"; + assert Strings.hasText(tierName) : "tierName must be not null and non-empty, but was [" + tierName + "]"; if (roles.contains(DiscoveryNodeRole.DATA_ROLE)) { // generic "data" roles are considered to have all tiers