Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add force_synthetic_source to GET #87536

Merged
merged 2 commits into from
Jun 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/api/get.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@
]
},
"params":{
"force_synthetic_source": {
"type": "boolean",
"description": "Should this request force synthetic _source? Use this to test if the mapping supports synthetic _source and to get a sense of the worst case performance. Fetches with this enabled will be slower the enabling synthetic source natively in the index.",
"visibility": "feature_flag",
"feature_flag": "es.index_mode_feature_flag_registered"
},
"stored_fields":{
"type":"list",
"description":"A comma-separated list of stored fields to return in the response"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,94 @@ keyword:
- match:
_source:
kwd: foo

---
force_synthetic_source_ok:
- skip:
version: " - 8.3.99"
reason: introduced in 8.4.0

- do:
indices.create:
index: test
body:
mappings:
_source:
synthetic: false
properties:
obj:
properties:
kwd:
type: keyword

- do:
index:
index: test
id: 1
refresh: true
body:
obj.kwd: foo

# When _source is used in the fetch the original _source is perfect
- do:
get:
index: test
id: 1
- match:
_source:
obj.kwd: foo

# When we force synthetic source dots in field names get turned into objects
- do:
get:
index: test
id: 1
force_synthetic_source: true
- match:
_source:
obj:
kwd: foo

---
force_synthetic_source_bad_mapping:
- skip:
version: " - 8.3.99"
reason: introduced in 8.4.0

- do:
indices.create:
index: test
body:
settings:
number_of_shards: 1 # Use a single shard to get consistent error messages
mappings:
_source:
synthetic: false
properties:
text:
type: text

- do:
index:
index: test
id: 1
refresh: true
body:
text: foo

# When _source is used in the fetch the original _source is perfect
- do:
get:
index: test
id: 1
- match:
_source:
text: foo

# Forcing synthetic source fails because the mapping is invalid
- do:
catch: bad_request
get:
index: test
id: 1
force_synthetic_source: true
41 changes: 41 additions & 0 deletions server/src/main/java/org/elasticsearch/action/get/GetRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.common.lucene.uid.Versions;
import org.elasticsearch.index.VersionType;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.SourceLoader;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;

import java.io.IOException;
Expand Down Expand Up @@ -55,6 +56,14 @@ public class GetRequest extends SingleShardRequest<GetRequest> implements Realti
private VersionType versionType = VersionType.INTERNAL;
private long version = Versions.MATCH_ANY;

/**
* Should this request force {@link SourceLoader.Synthetic synthetic source}?
* Use this to test if the mapping supports synthetic _source and to get a sense
* of the worst case performance. Fetches with this enabled will be slower the
* enabling synthetic source natively in the index.
*/
private boolean forceSyntheticSource = false;

public GetRequest() {}

GetRequest(StreamInput in) throws IOException {
Expand All @@ -72,6 +81,11 @@ public GetRequest() {}
this.versionType = VersionType.fromValue(in.readByte());
this.version = in.readLong();
fetchSourceContext = in.readOptionalWriteable(FetchSourceContext::readFrom);
if (in.getVersion().onOrAfter(Version.V_8_4_0)) {
forceSyntheticSource = in.readBoolean();
} else {
forceSyntheticSource = false;
}
}

@Override
Expand All @@ -90,6 +104,13 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeByte(versionType.getValue());
out.writeLong(version);
out.writeOptionalWriteable(fetchSourceContext);
if (out.getVersion().onOrAfter(Version.V_8_4_0)) {
out.writeBoolean(forceSyntheticSource);
} else {
if (forceSyntheticSource) {
throw new IllegalArgumentException("force_synthetic_source is not supported before 8.4.0");
}
}
}

/**
Expand Down Expand Up @@ -242,6 +263,26 @@ public VersionType versionType() {
return this.versionType;
}

/**
* Should this request force {@link SourceLoader.Synthetic synthetic source}?
* Use this to test if the mapping supports synthetic _source and to get a sense
* of the worst case performance. Fetches with this enabled will be slower the
* enabling synthetic source natively in the index.
*/
public void setForceSyntheticSource(boolean forceSyntheticSource) {
this.forceSyntheticSource = forceSyntheticSource;
}

/**
* Should this request force {@link SourceLoader.Synthetic synthetic source}?
* Use this to test if the mapping supports synthetic _source and to get a sense
* of the worst case performance. Fetches with this enabled will be slower the
* enabling synthetic source natively in the index.
*/
public boolean isForceSyntheticSource() {
return forceSyntheticSource;
}

@Override
public String toString() {
return "get [" + index + "][" + id + "]: routing [" + routing + "]";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ protected GetResponse shardOperation(GetRequest request, ShardId shardId) throws
request.realtime(),
request.version(),
request.versionType(),
request.fetchSourceContext()
request.fetchSourceContext(),
request.isForceSyntheticSource()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was making me think that maybe we should add forceSynthetic as a member on FetchSourceContext, but it looks as though that is used in loads of other places so it would end up being a much bigger change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I thought about it and didn't like.

);
return new GetResponse(result);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,15 @@ protected MultiGetShardResponse shardOperation(MultiGetShardRequest request, Sha
MultiGetRequest.Item item = request.items.get(i);
try {
GetResult getResult = indexShard.getService()
.get(item.id(), item.storedFields(), request.realtime(), item.version(), item.versionType(), item.fetchSourceContext());
.get(
item.id(),
item.storedFields(),
request.realtime(),
item.version(),
item.versionType(),
item.fetchSourceContext(),
false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be enabled in a follow-up, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be request.isForceSyntheticSource() I think.

);
response.add(request.locations.get(i), new GetResponse(getResult));
} catch (RuntimeException e) {
if (TransportActions.isShardNotAvailableException(e)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,9 @@ public boolean isForceSyntheticSource() {

/**
* Should this request force {@link SourceLoader.Synthetic synthetic source}?
* Use this to test if the mapping supports synthetic _source and to get a sense
* of the worst case performance. Fetches with this enabled will be slower the
* enabling synthetic source natively in the index.
*/
public void setForceSyntheticSource(boolean forceSyntheticSource) {
this.forceSyntheticSource = forceSyntheticSource;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.elasticsearch.index.mapper.MappingLookup;
import org.elasticsearch.index.mapper.RoutingFieldMapper;
import org.elasticsearch.index.mapper.SourceFieldMapper;
import org.elasticsearch.index.mapper.SourceLoader;
import org.elasticsearch.index.shard.AbstractIndexShardComponent;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
Expand Down Expand Up @@ -70,9 +71,20 @@ public GetResult get(
boolean realtime,
long version,
VersionType versionType,
FetchSourceContext fetchSourceContext
FetchSourceContext fetchSourceContext,
boolean forceSyntheticSource
) throws IOException {
return get(id, gFields, realtime, version, versionType, UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM, fetchSourceContext);
return get(
id,
gFields,
realtime,
version,
versionType,
UNASSIGNED_SEQ_NO,
UNASSIGNED_PRIMARY_TERM,
fetchSourceContext,
forceSyntheticSource
);
}

private GetResult get(
Expand All @@ -83,12 +95,23 @@ private GetResult get(
VersionType versionType,
long ifSeqNo,
long ifPrimaryTerm,
FetchSourceContext fetchSourceContext
FetchSourceContext fetchSourceContext,
boolean forceSyntheticSource
) throws IOException {
currentMetric.inc();
try {
long now = System.nanoTime();
GetResult getResult = innerGet(id, gFields, realtime, version, versionType, ifSeqNo, ifPrimaryTerm, fetchSourceContext);
GetResult getResult = innerGet(
id,
gFields,
realtime,
version,
versionType,
ifSeqNo,
ifPrimaryTerm,
fetchSourceContext,
forceSyntheticSource
);

if (getResult.isExists()) {
existsMetric.inc(System.nanoTime() - now);
Expand All @@ -110,7 +133,8 @@ public GetResult getForUpdate(String id, long ifSeqNo, long ifPrimaryTerm) throw
VersionType.INTERNAL,
ifSeqNo,
ifPrimaryTerm,
FetchSourceContext.FETCH_SOURCE
FetchSourceContext.FETCH_SOURCE,
false
);
}

Expand All @@ -131,7 +155,7 @@ public GetResult get(Engine.GetResult engineGetResult, String id, String[] field
try {
long now = System.nanoTime();
fetchSourceContext = normalizeFetchSourceContent(fetchSourceContext, fields);
GetResult getResult = innerGetLoadFromStoredFields(id, fields, fetchSourceContext, engineGetResult);
GetResult getResult = innerGetFetch(id, fields, fetchSourceContext, engineGetResult, false);
if (getResult.isExists()) {
existsMetric.inc(System.nanoTime() - now);
} else {
Expand Down Expand Up @@ -169,7 +193,8 @@ private GetResult innerGet(
VersionType versionType,
long ifSeqNo,
long ifPrimaryTerm,
FetchSourceContext fetchSourceContext
FetchSourceContext fetchSourceContext,
boolean forceSyntheticSource
) throws IOException {
fetchSourceContext = normalizeFetchSourceContent(fetchSourceContext, gFields);

Expand All @@ -189,17 +214,18 @@ private GetResult innerGet(

try {
// break between having loaded it from translog (so we only have _source), and having a document to load
return innerGetLoadFromStoredFields(id, gFields, fetchSourceContext, get);
return innerGetFetch(id, gFields, fetchSourceContext, get, forceSyntheticSource);
} finally {
get.close();
}
}

private GetResult innerGetLoadFromStoredFields(
private GetResult innerGetFetch(
String id,
String[] storedFields,
FetchSourceContext fetchSourceContext,
Engine.GetResult get
Engine.GetResult get,
boolean forceSyntheticSource
) throws IOException {
assert get.exists() : "method should only be called if document could be retrieved";

Expand Down Expand Up @@ -228,7 +254,10 @@ private GetResult innerGetLoadFromStoredFields(
} catch (IOException e) {
throw new ElasticsearchException("Failed to get id [" + id + "]", e);
}
source = mappingLookup.newSourceLoader().leaf(docIdAndVersion.reader).source(fieldVisitor, docIdAndVersion.docId);
SourceLoader loader = forceSyntheticSource
? new SourceLoader.Synthetic(mappingLookup.getMapping())
: mappingLookup.newSourceLoader();
source = loader.leaf(docIdAndVersion.reader).source(fieldVisitor, docIdAndVersion.docId);

// put stored fields into result objects
if (fieldVisitor.fields().isEmpty() == false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.client.internal.node.NodeClient;
import org.elasticsearch.common.Strings;
import org.elasticsearch.core.RestApiVersion;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.VersionType;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
Expand Down Expand Up @@ -78,6 +79,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
getRequest.versionType(VersionType.fromString(request.param("version_type"), getRequest.versionType()));

getRequest.fetchSourceContext(FetchSourceContext.parseFromRestRequest(request));
if (IndexSettings.isTimeSeriesModeEnabled() && request.paramAsBoolean("force_synthetic_source", false)) {
getRequest.setForceSyntheticSource(true);
}

return channel -> client.get(getRequest, new RestToXContentListener<GetResponse>(channel) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ private void runGetFromTranslogWithOptions(
Engine.IndexResult test2 = indexDoc(primary, "2", docToIndex, XContentType.JSON, "foobar");
assertTrue(primary.getEngine().refreshNeeded());
GetResult testGet2 = primary.getService()
.get("2", new String[] { "foo" }, true, 1, VersionType.INTERNAL, FetchSourceContext.FETCH_SOURCE);
.get("2", new String[] { "foo" }, true, 1, VersionType.INTERNAL, FetchSourceContext.FETCH_SOURCE, false);
assertEquals(new String(testGet2.source() == null ? new byte[0] : testGet2.source(), StandardCharsets.UTF_8), expectedResult);
assertTrue(testGet2.getFields().containsKey(RoutingFieldMapper.NAME));
assertTrue(testGet2.getFields().containsKey("foo"));
Expand All @@ -174,7 +174,8 @@ private void runGetFromTranslogWithOptions(
assertEquals(searcher.getIndexReader().maxDoc(), 3);
}

testGet2 = primary.getService().get("2", new String[] { "foo" }, true, 1, VersionType.INTERNAL, FetchSourceContext.FETCH_SOURCE);
testGet2 = primary.getService()
.get("2", new String[] { "foo" }, true, 1, VersionType.INTERNAL, FetchSourceContext.FETCH_SOURCE, false);
assertEquals(new String(testGet2.source() == null ? new byte[0] : testGet2.source(), StandardCharsets.UTF_8), expectedResult);
assertTrue(testGet2.getFields().containsKey(RoutingFieldMapper.NAME));
assertTrue(testGet2.getFields().containsKey("foo"));
Expand Down