Skip to content

Commit b558a66

Browse files
javannajavanna
authored
Merge branch 'main' into refactoring/remove_completion_extension
2 parents a279c37 + e210ea8 commit b558a66

File tree

12 files changed

+139
-85
lines changed

12 files changed

+139
-85
lines changed

‎modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RemoveProcessor.java‎

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,9 @@ public IngestDocument execute(IngestDocument document) {
5959
}
6060

6161
private void fieldsToRemoveProcessor(IngestDocument document) {
62-
// micro-optimization note: actual for-each loops here rather than a .forEach because it happens to be ~5% faster in benchmarks
63-
if (ignoreMissing) {
64-
for (TemplateScript.Factory field : fieldsToRemove) {
65-
removeWhenPresent(document, document.renderTemplate(field));
66-
}
67-
} else {
68-
for (TemplateScript.Factory field : fieldsToRemove) {
69-
document.removeField(document.renderTemplate(field));
70-
}
62+
// micro-optimization note: actual for-each loop here rather than a .forEach because it happens to be ~5% faster in benchmarks
63+
for (TemplateScript.Factory field : fieldsToRemove) {
64+
document.removeField(document.renderTemplate(field), ignoreMissing);
7165
}
7266
}
7367

@@ -76,13 +70,7 @@ private void fieldsToKeepProcessor(IngestDocument document) {
7670
.stream()
7771
.filter(documentField -> IngestDocument.Metadata.isMetadata(documentField) == false)
7872
.filter(documentField -> shouldKeep(documentField, fieldsToKeep, document) == false)
79-
.forEach(documentField -> removeWhenPresent(document, documentField));
80-
}
81-
82-
private static void removeWhenPresent(IngestDocument document, String documentField) {
83-
if (document.hasField(documentField)) {
84-
document.removeField(documentField);
85-
}
73+
.forEach(documentField -> document.removeField(documentField, true));
8674
}
8775

8876
static boolean shouldKeep(String documentField, List<TemplateScript.Factory> fieldsToKeep, IngestDocument document) {

‎server/src/main/java/org/elasticsearch/action/get/TransportGetAction.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,18 +167,18 @@ protected Writeable.Reader<GetResponse> getResponseReader() {
167167
}
168168

169169
@Override
170-
protected Executor getExecutor(GetRequest request, ShardId shardId) {
170+
protected Executor getExecutor(ShardId shardId) {
171171
final ClusterState clusterState = clusterService.state();
172172
if (projectResolver.getProjectMetadata(clusterState).getIndexSafe(shardId.getIndex()).isSystem()) {
173173
return threadPool.executor(executorSelector.executorForGet(shardId.getIndexName()));
174174
} else {
175-
return super.getExecutor(request, shardId);
175+
return super.getExecutor(shardId);
176176
}
177177
}
178178

179179
private void asyncGet(GetRequest request, ShardId shardId, ActionListener<GetResponse> listener) throws IOException {
180180
if (request.refresh() && request.realtime() == false) {
181-
getExecutor(request, shardId).execute(ActionRunnable.wrap(listener, l -> {
181+
getExecutor(shardId).execute(ActionRunnable.wrap(listener, l -> {
182182
var indexShard = getIndexShard(shardId);
183183
indexShard.externalRefresh("refresh_flag_get", l.map(r -> shardOperation(request, shardId)));
184184
}));
@@ -300,7 +300,7 @@ private void tryGetFromTranslog(GetRequest request, IndexShard indexShard, Disco
300300
indexShard.waitForPrimaryTermAndGeneration(r.primaryTerm(), r.segmentGeneration(), termAndGenerationListener);
301301
}
302302
}
303-
}), TransportGetFromTranslogAction.Response::new, getExecutor(request, shardId))
303+
}), TransportGetFromTranslogAction.Response::new, getExecutor(shardId))
304304
);
305305
}
306306

‎server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -156,12 +156,12 @@ protected MultiGetShardResponse shardOperation(MultiGetShardRequest request, Sha
156156
}
157157

158158
@Override
159-
protected Executor getExecutor(MultiGetShardRequest request, ShardId shardId) {
159+
protected Executor getExecutor(ShardId shardId) {
160160
final ClusterState clusterState = clusterService.state();
161161
if (projectResolver.getProjectMetadata(clusterState).index(shardId.getIndex()).isSystem()) {
162162
return threadPool.executor(executorSelector.executorForGet(shardId.getIndexName()));
163163
} else {
164-
return super.getExecutor(request, shardId);
164+
return super.getExecutor(shardId);
165165
}
166166
}
167167

@@ -290,7 +290,7 @@ private void tryShardMultiGetFromTranslog(
290290
assert r.primaryTerm() > Engine.UNKNOWN_PRIMARY_TERM;
291291
final ActionListener<Long> termAndGenerationListener = ContextPreservingActionListener.wrapPreservingContext(
292292
listener.delegateFailureAndWrap(
293-
(ll, aLong) -> getExecutor(request, shardId).execute(
293+
(ll, aLong) -> getExecutor(shardId).execute(
294294
ActionRunnable.supply(ll, () -> handleLocalGets(request, r.multiGetShardResponse(), shardId))
295295
)
296296
),
@@ -299,7 +299,7 @@ private void tryShardMultiGetFromTranslog(
299299
indexShard.waitForPrimaryTermAndGeneration(r.primaryTerm(), r.segmentGeneration(), termAndGenerationListener);
300300
}
301301
}
302-
}), TransportShardMultiGetFomTranslogAction.Response::new, getExecutor(request, shardId))
302+
}), TransportShardMultiGetFomTranslogAction.Response::new, getExecutor(shardId))
303303
);
304304
}
305305

@@ -353,7 +353,7 @@ private void getAndAddToResponse(
353353
private void asyncShardMultiGet(MultiGetShardRequest request, ShardId shardId, ActionListener<MultiGetShardResponse> listener)
354354
throws IOException {
355355
if (request.refresh() && request.realtime() == false) {
356-
getExecutor(request, shardId).execute(ActionRunnable.wrap(listener, l -> {
356+
getExecutor(shardId).execute(ActionRunnable.wrap(listener, l -> {
357357
var indexShard = getIndexShard(shardId);
358358
indexShard.externalRefresh("refresh_flag_mget", l.map(r -> shardOperation(request, shardId)));
359359
}));

‎server/src/main/java/org/elasticsearch/action/support/single/shard/TransportSingleShardAction.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ protected void doExecute(Task task, Request request, ActionListener<Response> li
114114
protected abstract Response shardOperation(Request request, ShardId shardId) throws IOException;
115115

116116
protected void asyncShardOperation(Request request, ShardId shardId, ActionListener<Response> listener) throws IOException {
117-
getExecutor(request, shardId).execute(ActionRunnable.supplyAndDecRef(listener, () -> shardOperation(request, shardId)));
117+
getExecutor(shardId).execute(ActionRunnable.supplyAndDecRef(listener, () -> shardOperation(request, shardId)));
118118
}
119119

120120
protected abstract Writeable.Reader<Response> getResponseReader();
@@ -300,7 +300,7 @@ public String concreteIndex() {
300300
}
301301
}
302302

303-
protected Executor getExecutor(Request request, ShardId shardId) {
303+
protected Executor getExecutor(ShardId shardId) {
304304
return executor;
305305
}
306306
}

‎server/src/main/java/org/elasticsearch/ingest/IngestDocument.java‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -317,15 +317,29 @@ public boolean hasField(String path, boolean failOutOfRange) {
317317

318318
/**
319319
* Removes the field identified by the provided path.
320+
*
320321
* @param path the path of the field to be removed
321322
* @throws IllegalArgumentException if the path is null, empty, invalid or if the field doesn't exist.
322323
*/
323324
public void removeField(String path) {
325+
removeField(path, false);
326+
}
327+
328+
/**
329+
* Removes the field identified by the provided path.
330+
*
331+
* @param path the path of the field to be removed
332+
* @param ignoreMissing The flag to determine whether to throw an exception when `path` is not found in the document.
333+
* @throws IllegalArgumentException if the path is null, empty, or invalid; or if the field doesn't exist (and ignoreMissing is false).
334+
*/
335+
public void removeField(String path, boolean ignoreMissing) {
324336
final FieldPath fieldPath = FieldPath.of(path);
325337
Object context = fieldPath.initialContext(this);
326338
ResolveResult result = resolve(fieldPath.pathElements, fieldPath.pathElements.length - 1, path, context);
327339
if (result.wasSuccessful) {
328340
context = result.resolvedObject;
341+
} else if (ignoreMissing) {
342+
return; // nothing was found, so there's nothing to remove :shrug:
329343
} else {
330344
throw new IllegalArgumentException(result.errorMessage);
331345
}
@@ -336,13 +350,13 @@ public void removeField(String path) {
336350
} else if (context instanceof IngestCtxMap map) { // optimization: handle IngestCtxMap separately from Map
337351
if (map.containsKey(leafKey)) {
338352
map.remove(leafKey);
339-
} else {
353+
} else if (ignoreMissing == false) {
340354
throw new IllegalArgumentException(Errors.notPresent(path, leafKey));
341355
}
342356
} else if (context instanceof Map<?, ?> map) {
343357
if (map.containsKey(leafKey)) {
344358
map.remove(leafKey);
345-
} else {
359+
} else if (ignoreMissing == false) {
346360
throw new IllegalArgumentException(Errors.notPresent(path, leafKey));
347361
}
348362
} else if (context instanceof List<?> list) {
@@ -353,11 +367,13 @@ public void removeField(String path) {
353367
throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e);
354368
}
355369
if (index < 0 || index >= list.size()) {
356-
throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
370+
if (ignoreMissing == false) {
371+
throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
372+
}
357373
} else {
358374
list.remove(index);
359375
}
360-
} else {
376+
} else if (ignoreMissing == false) {
361377
throw new IllegalArgumentException(Errors.cannotRemove(path, leafKey, context));
362378
}
363379
}

‎server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,23 @@ public void testRemoveField() {
850850
assertThat(document.getIngestMetadata().size(), equalTo(0));
851851
}
852852

853+
public void testRemoveFieldIgnoreMissing() {
854+
document.removeField("foo", randomBoolean());
855+
assertThat(document.getSourceAndMetadata().size(), equalTo(10));
856+
assertThat(document.getSourceAndMetadata().containsKey("foo"), equalTo(false));
857+
document.removeField("_index", randomBoolean());
858+
assertThat(document.getSourceAndMetadata().size(), equalTo(9));
859+
assertThat(document.getSourceAndMetadata().containsKey("_index"), equalTo(false));
860+
861+
// if ignoreMissing is false, we throw an exception for values that aren't found
862+
IllegalArgumentException e;
863+
e = expectThrows(IllegalArgumentException.class, () -> document.removeField("fizz.some.nonsense", false));
864+
assertThat(e.getMessage(), is("field [some] not present as part of path [fizz.some.nonsense]"));
865+
866+
// but no exception is thrown if ignoreMissing is true
867+
document.removeField("fizz.some.nonsense", true);
868+
}
869+
853870
public void testRemoveInnerField() {
854871
document.removeField("fizz.buzz");
855872
assertThat(document.getSourceAndMetadata().size(), equalTo(11));

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/inference/action/GetInferenceModelAction.java‎

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -44,23 +44,15 @@ public static class Request extends AcknowledgedRequest<GetInferenceModelAction.
4444
// no effect when getting a single model
4545
private final boolean persistDefaultConfig;
4646

47-
// For testing only, retrieves the minimal config from the cluster state.
48-
private final boolean returnMinimalConfig;
49-
5047
public Request(String inferenceEntityId, TaskType taskType) {
5148
this(inferenceEntityId, taskType, PERSIST_DEFAULT_CONFIGS);
5249
}
5350

5451
public Request(String inferenceEntityId, TaskType taskType, boolean persistDefaultConfig) {
55-
this(inferenceEntityId, taskType, persistDefaultConfig, false);
56-
}
57-
58-
public Request(String inferenceEntityId, TaskType taskType, boolean persistDefaultConfig, boolean returnMinimalConfig) {
5952
super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
6053
this.inferenceEntityId = Objects.requireNonNull(inferenceEntityId);
6154
this.taskType = Objects.requireNonNull(taskType);
6255
this.persistDefaultConfig = persistDefaultConfig;
63-
this.returnMinimalConfig = returnMinimalConfig;
6456
}
6557

6658
public Request(StreamInput in) throws IOException {
@@ -72,13 +64,6 @@ public Request(StreamInput in) throws IOException {
7264
} else {
7365
this.persistDefaultConfig = PERSIST_DEFAULT_CONFIGS;
7466
}
75-
76-
if (in.getTransportVersion().onOrAfter(TransportVersions.INFERENCE_MODEL_REGISTRY_METADATA)) {
77-
this.returnMinimalConfig = in.readBoolean();
78-
} else {
79-
this.returnMinimalConfig = false;
80-
}
81-
8267
}
8368

8469
public String getInferenceEntityId() {
@@ -93,10 +78,6 @@ public boolean isPersistDefaultConfig() {
9378
return persistDefaultConfig;
9479
}
9580

96-
public boolean isReturnMinimalConfig() {
97-
return returnMinimalConfig;
98-
}
99-
10081
@Override
10182
public void writeTo(StreamOutput out) throws IOException {
10283
super.writeTo(out);
@@ -105,10 +86,6 @@ public void writeTo(StreamOutput out) throws IOException {
10586
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_16_0)) {
10687
out.writeBoolean(this.persistDefaultConfig);
10788
}
108-
109-
if (out.getTransportVersion().onOrAfter(TransportVersions.INFERENCE_MODEL_REGISTRY_METADATA)) {
110-
out.writeBoolean(returnMinimalConfig);
111-
}
11289
}
11390

11491
@Override
@@ -118,13 +95,12 @@ public boolean equals(Object o) {
11895
Request request = (Request) o;
11996
return Objects.equals(inferenceEntityId, request.inferenceEntityId)
12097
&& taskType == request.taskType
121-
&& persistDefaultConfig == request.persistDefaultConfig
122-
&& returnMinimalConfig == request.returnMinimalConfig;
98+
&& persistDefaultConfig == request.persistDefaultConfig;
12399
}
124100

125101
@Override
126102
public int hashCode() {
127-
return Objects.hash(inferenceEntityId, taskType, persistDefaultConfig, returnMinimalConfig);
103+
return Objects.hash(inferenceEntityId, taskType, persistDefaultConfig);
128104
}
129105
}
130106

‎x-pack/plugin/inference/qa/inference-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/DefaultEndPointsIT.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.concurrent.CountDownLatch;
2626

2727
import static org.hamcrest.Matchers.empty;
28+
import static org.hamcrest.Matchers.equalTo;
2829
import static org.hamcrest.Matchers.hasSize;
2930
import static org.hamcrest.Matchers.is;
3031
import static org.hamcrest.Matchers.oneOf;
@@ -62,6 +63,25 @@ public void testGet() throws IOException {
6263
assertDefaultRerankConfig(rerankModel);
6364
}
6465

66+
public void testDefaultModels() throws IOException {
67+
var elserModel = getModel(ElasticsearchInternalService.DEFAULT_ELSER_ID);
68+
assertDefaultElserConfig(elserModel);
69+
70+
var e5Model = getModel(ElasticsearchInternalService.DEFAULT_E5_ID);
71+
assertDefaultE5Config(e5Model);
72+
73+
var rerankModel = getModel(ElasticsearchInternalService.DEFAULT_RERANK_ID);
74+
assertDefaultRerankConfig(rerankModel);
75+
76+
putModel("my-model", mockCompletionServiceModelConfig(TaskType.SPARSE_EMBEDDING));
77+
var registeredModels = getMinimalConfigs();
78+
assertThat(registeredModels.size(), equalTo(1));
79+
assertTrue(registeredModels.containsKey("my-model"));
80+
assertFalse(registeredModels.containsKey(ElasticsearchInternalService.DEFAULT_E5_ID));
81+
assertFalse(registeredModels.containsKey(ElasticsearchInternalService.DEFAULT_ELSER_ID));
82+
assertFalse(registeredModels.containsKey(ElasticsearchInternalService.DEFAULT_RERANK_ID));
83+
}
84+
6585
@SuppressWarnings("unchecked")
6686
public void testInferDeploysDefaultElser() throws IOException {
6787
var model = getModel(ElasticsearchInternalService.DEFAULT_ELSER_ID);

‎x-pack/plugin/inference/qa/inference-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/InferenceBaseRestTest.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.common.settings.SecureString;
1717
import org.elasticsearch.common.settings.Settings;
1818
import org.elasticsearch.common.util.concurrent.ThreadContext;
19+
import org.elasticsearch.common.xcontent.support.XContentMapValues;
1920
import org.elasticsearch.core.Nullable;
2021
import org.elasticsearch.inference.TaskType;
2122
import org.elasticsearch.test.cluster.ElasticsearchCluster;
@@ -514,4 +515,13 @@ protected Map<String, Object> getTrainedModel(String inferenceEntityId) throws I
514515
assertStatusOkOrCreated(response);
515516
return entityAsMap(response);
516517
}
518+
519+
@SuppressWarnings("unchecked")
520+
protected Map<String, Map<String, Object>> getMinimalConfigs() throws IOException {
521+
var endpoint = "_cluster/state?filter_path=metadata.model_registry";
522+
var request = new Request("GET", endpoint);
523+
var response = client().performRequest(request);
524+
assertOK(response);
525+
return (Map<String, Map<String, Object>>) XContentMapValues.extractValue("metadata.model_registry.models", entityAsMap(response));
526+
}
517527
}

‎x-pack/plugin/inference/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/xpack/application/InferenceUpgradeTestCase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ protected Map<String, Object> get(TaskType taskType, String inferenceId) throws
8989
}
9090

9191
@SuppressWarnings("unchecked")
92-
protected Map<String, Map<String, Object>> getMinimalConfig() throws IOException {
92+
protected Map<String, Map<String, Object>> getMinimalConfigs() throws IOException {
9393
var endpoint = "_cluster/state?filter_path=metadata.model_registry";
9494
var request = new Request("GET", endpoint);
9595
var response = client().performRequest(request);

0 commit comments

Comments
 (0)