Skip to content

Commit adcf328

Browse files
gaobinlonggaobinlong
committed
Fix updating search backpressure settings crashing the cluster
Signed-off-by: Gao Binlong <[email protected]>
1 parent 2b84305 commit adcf328

File tree

8 files changed

+183
-3
lines changed

8 files changed

+183
-3
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
8080
- Fix indexing error when flat_object field is explicitly null ([#15375](https://github.com/opensearch-project/OpenSearch/pull/15375))
8181
- Fix split response processor not included in allowlist ([#15393](https://github.com/opensearch-project/OpenSearch/pull/15393))
8282
- Fix unchecked cast in dynamic action map getter ([#15394](https://github.com/opensearch-project/OpenSearch/pull/15394))
83+
- Fix updating search backpressure settings crashing the cluster
8384

8485
### Security
8586

‎rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,3 +98,116 @@
9898
- match: {error.root_cause.0.type: "illegal_argument_exception"}
9999
- match: { error.root_cause.0.reason: "Invalid SearchBackpressureMode: monitor-only" }
100100
- match: { status: 400 }
101+
102+
103+
---
104+
"Test setting search backpressure cancellation settings":
105+
- skip:
106+
version: "- 2.3.99"
107+
reason: "Introduced in 2.4.0"
108+
109+
- do:
110+
cluster.put_settings:
111+
body:
112+
transient:
113+
search_backpressure.search_task.cancellation_burst: 11
114+
- is_true: acknowledged
115+
116+
- do:
117+
cluster.get_settings:
118+
flat_settings: false
119+
- match: {transient.search_backpressure.search_task.cancellation_burst: "11"}
120+
121+
- do:
122+
cluster.put_settings:
123+
body:
124+
transient:
125+
search_backpressure.search_task.cancellation_rate: 0.1
126+
- is_true: acknowledged
127+
128+
- do:
129+
cluster.get_settings:
130+
flat_settings: false
131+
- match: {transient.search_backpressure.search_task.cancellation_rate: "0.1"}
132+
133+
- do:
134+
cluster.put_settings:
135+
body:
136+
transient:
137+
search_backpressure.search_task.cancellation_ratio: 0.2
138+
- is_true: acknowledged
139+
140+
- do:
141+
cluster.get_settings:
142+
flat_settings: false
143+
- match: {transient.search_backpressure.search_task.cancellation_ratio: "0.2"}
144+
145+
- do:
146+
cluster.put_settings:
147+
body:
148+
transient:
149+
search_backpressure.search_shard_task.cancellation_burst: 12
150+
- is_true: acknowledged
151+
152+
- do:
153+
cluster.get_settings:
154+
flat_settings: false
155+
- match: {transient.search_backpressure.search_shard_task.cancellation_burst: "12"}
156+
157+
- do:
158+
cluster.put_settings:
159+
body:
160+
transient:
161+
search_backpressure.search_shard_task.cancellation_rate: 0.3
162+
- is_true: acknowledged
163+
164+
- do:
165+
cluster.get_settings:
166+
flat_settings: false
167+
- match: {transient.search_backpressure.search_shard_task.cancellation_rate: "0.3"}
168+
169+
- do:
170+
cluster.put_settings:
171+
body:
172+
transient:
173+
search_backpressure.search_shard_task.cancellation_ratio: 0.4
174+
- is_true: acknowledged
175+
176+
- do:
177+
cluster.get_settings:
178+
flat_settings: false
179+
- match: {transient.search_backpressure.search_shard_task.cancellation_ratio: "0.4"}
180+
181+
---
182+
"Test setting invalid search backpressure cancellation_rate and cancellation_ratio":
183+
- skip:
184+
version: "- 2.99.99"
185+
reason: "Fixed in 3.0.0"
186+
187+
- do:
188+
catch: /search_backpressure.search_task.cancellation_rate must be greater than zero/
189+
cluster.put_settings:
190+
body:
191+
transient:
192+
search_backpressure.search_task.cancellation_rate: 0.0
193+
194+
- do:
195+
catch: /search_backpressure.search_task.cancellation_ratio must be greater than zero/
196+
cluster.put_settings:
197+
body:
198+
transient:
199+
search_backpressure.search_task.cancellation_ratio: 0.0
200+
201+
- do:
202+
catch: /search_backpressure.search_shard_task.cancellation_rate must be greater than zero/
203+
cluster.put_settings:
204+
body:
205+
transient:
206+
search_backpressure.search_shard_task.cancellation_rate: 0.0
207+
208+
- do:
209+
catch: /search_backpressure.search_shard_task.cancellation_ratio must be greater than zero/
210+
cluster.put_settings:
211+
body:
212+
transient:
213+
search_backpressure.search_shard_task.cancellation_ratio: 0.0

‎server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,14 +160,16 @@ public SearchBackpressureService(
160160
timeNanosSupplier,
161161
getSettings().getSearchTaskSettings().getCancellationRateNanos(),
162162
getSettings().getSearchTaskSettings().getCancellationBurst(),
163-
getSettings().getSearchTaskSettings().getCancellationRatio()
163+
getSettings().getSearchTaskSettings().getCancellationRatio(),
164+
getSettings().getSearchTaskSettings().getCancellationRate()
164165
),
165166
SearchShardTask.class,
166167
new SearchBackpressureState(
167168
timeNanosSupplier,
168169
getSettings().getSearchShardTaskSettings().getCancellationRateNanos(),
169170
getSettings().getSearchShardTaskSettings().getCancellationBurst(),
170-
getSettings().getSearchShardTaskSettings().getCancellationRatio()
171+
getSettings().getSearchShardTaskSettings().getCancellationRatio(),
172+
getSettings().getSearchShardTaskSettings().getCancellationRate()
171173
)
172174
);
173175
this.settings.getSearchTaskSettings().addListener(searchBackpressureStates.get(SearchTask.class));

‎server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,15 @@ public class SearchBackpressureState implements CancellationSettingsListener {
4343
LongSupplier timeNanosSupplier,
4444
double cancellationRateNanos,
4545
double cancellationBurst,
46-
double cancellationRatio
46+
double cancellationRatio,
47+
double cancellationRate
4748
) {
4849
rateLimiter = new AtomicReference<>(new TokenBucket(timeNanosSupplier, cancellationRateNanos, cancellationBurst));
4950
ratioLimiter = new AtomicReference<>(new TokenBucket(this::getCompletionCount, cancellationRatio, cancellationBurst));
5051
this.timeNanosSupplier = timeNanosSupplier;
5152
this.cancellationBurst = cancellationBurst;
53+
this.cancellationRatio = cancellationRatio;
54+
this.cancellationRate = cancellationRate;
5255
}
5356

5457
public long getCompletionCount() {

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ private static class Defaults {
6363
Defaults.CANCELLATION_RATIO,
6464
0.0,
6565
1.0,
66+
value -> {
67+
if (value <= 0.0) {
68+
throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be greater than zero");
69+
}
70+
},
6671
Setting.Property.Deprecated,
6772
Setting.Property.Dynamic,
6873
Setting.Property.NodeScope
@@ -79,6 +84,12 @@ private static class Defaults {
7984
"search_backpressure.cancellation_rate",
8085
Defaults.CANCELLATION_RATE,
8186
0.0,
87+
Double.MAX_VALUE,
88+
value -> {
89+
if (value <= 0.0) {
90+
throw new IllegalArgumentException("search_backpressure.cancellation_rate must be greater than zero");
91+
}
92+
},
8293
Setting.Property.Deprecated,
8394
Setting.Property.Dynamic,
8495
Setting.Property.NodeScope

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ private static class Defaults {
5050
SearchBackpressureSettings.SETTING_CANCELLATION_RATIO,
5151
0.0,
5252
1.0,
53+
value -> {
54+
if (value <= 0.0) {
55+
throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be greater than zero");
56+
}
57+
},
5358
Setting.Property.Dynamic,
5459
Setting.Property.NodeScope
5560
);
@@ -63,6 +68,12 @@ private static class Defaults {
6368
"search_backpressure.search_shard_task.cancellation_rate",
6469
SearchBackpressureSettings.SETTING_CANCELLATION_RATE,
6570
0.0,
71+
Double.MAX_VALUE,
72+
value -> {
73+
if (value <= 0.0) {
74+
throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_rate must be greater than zero");
75+
}
76+
},
6677
Setting.Property.Dynamic,
6778
Setting.Property.NodeScope
6879
);

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ private static class Defaults {
5151
Defaults.CANCELLATION_RATIO,
5252
0.0,
5353
1.0,
54+
value -> {
55+
if (value <= 0.0) {
56+
throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be greater than zero");
57+
}
58+
},
5459
Setting.Property.Dynamic,
5560
Setting.Property.NodeScope
5661
);
@@ -64,6 +69,12 @@ private static class Defaults {
6469
"search_backpressure.search_task.cancellation_rate",
6570
Defaults.CANCELLATION_RATE,
6671
0.0,
72+
Double.MAX_VALUE,
73+
value -> {
74+
if (value <= 0.0) {
75+
throw new IllegalArgumentException("search_backpressure.search_task.cancellation_rate must be greater than zero");
76+
}
77+
},
6778
Setting.Property.Dynamic,
6879
Setting.Property.NodeScope
6980
);

‎server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,32 @@ public void testSearchBackpressureSettingValidateInvalidMode() {
3737
() -> new SearchBackpressureSettings(settings, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
3838
);
3939
}
40+
41+
public void testInvalidCancellationRate() {
42+
Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_rate", 0.0).build();
43+
assertThrows(
44+
IllegalArgumentException.class,
45+
() -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
46+
);
47+
48+
Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_rate", 0.0).build();
49+
assertThrows(
50+
IllegalArgumentException.class,
51+
() -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
52+
);
53+
}
54+
55+
public void testInvalidCancellationRatio() {
56+
Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_ratio", 0.0).build();
57+
assertThrows(
58+
IllegalArgumentException.class,
59+
() -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
60+
);
61+
62+
Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_ratio", 0.0).build();
63+
assertThrows(
64+
IllegalArgumentException.class,
65+
() -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
66+
);
67+
}
4068
}

0 commit comments

Comments
 (0)