Skip to content

Store Template's mappings as bytes for disk serialization #78746

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Oct 19, 2021
Merged

Store Template's mappings as bytes for disk serialization #78746

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a17e437
Store mappings as bytes for disk serialization
probakowski Oct 6, 2021
1bbccb6
Test fix
probakowski Oct 6, 2021
d490007
rollback changes
probakowski Oct 6, 2021
0880079
fix tests
probakowski Oct 6, 2021
25bb42d
Merge remote-tracking branch 'origin/master' into template-fix
probakowski Oct 6, 2021
256618f
fix serialization with json
probakowski Oct 6, 2021
f36868c
change serialization
probakowski Oct 7, 2021
1473a23
Merge branch 'master' into template-fix
probakowski Oct 7, 2021
f0457af
rework
probakowski Oct 11, 2021
a257b20
Merge remote-tracking branch 'origin/master' into template-fix
probakowski Oct 11, 2021
0d29911
fix compilation
probakowski Oct 11, 2021
579e139
Merge remote-tracking branch 'origin/master' into template-fix
probakowski Oct 11, 2021
fda4aa9
Merge branch 'master' into template-fix
elasticmachine Oct 12, 2021
1758f8d
parse ComposableTemplateMetadata with node tool
probakowski Oct 14, 2021
e1bd326
Merge branch 'master' into template-fix
elasticmachine Oct 14, 2021
58a691a
parse ComponentTemplate in node tool
probakowski Oct 14, 2021
5b0408f
rollback unneeded part
probakowski Oct 14, 2021
f45b195
rollback unneeded part
probakowski Oct 14, 2021
77f744d
rollback unneeded part
probakowski Oct 14, 2021
81265a8
Merge branch 'master' into template-fix
elasticmachine Oct 14, 2021
d8f4f86
Merge branch 'master' into template-fix
elasticmachine Oct 19, 2021
e4ccdaa
Merge branch 'master' into template-fix
elasticmachine Oct 19, 2021
58016b3
Merge branch 'master' into template-fix
elasticmachine Oct 19, 2021
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
3 changes: 2 additions & 1 deletion ...el/src/test/java/org/elasticsearch/client/indices/GetComponentTemplatesResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.cluster.metadata.Template;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ESTestCase;

Expand Down Expand Up @@ -79,7 +80,7 @@ private static void toXContent(GetComponentTemplatesResponse response, XContentB
builder.startObject();
builder.field("name", e.getKey());
builder.field("component_template");
e.getValue().toXContent(builder, null);
e.getValue().toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
}
builder.endArray();
Expand Down
3 changes: 2 additions & 1 deletion .../test/java/org/elasticsearch/client/indices/GetComposableIndexTemplatesResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
package org.elasticsearch.client.indices;

import org.elasticsearch.cluster.metadata.ComposableIndexTemplate;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ESTestCase;

Expand Down Expand Up @@ -53,7 +54,7 @@ private static void toXContent(GetComposableIndexTemplatesResponse response, XCo
builder.startObject();
builder.field("name", e.getKey());
builder.field("index_template");
e.getValue().toXContent(builder, null);
e.getValue().toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
}
builder.endArray();
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/cluster/metadata/ComponentTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public String toString() {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field(TEMPLATE.getPreferredName(), this.template);
builder.field(TEMPLATE.getPreferredName(), this.template, params);
if (this.version != null) {
builder.field(VERSION.getPreferredName(), this.version);
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/cluster/metadata/ComponentTemplateMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public static ComponentTemplateMetadata fromXContent(XContentParser parser) thro
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(COMPONENT_TEMPLATE.getPreferredName());
for (Map.Entry<String, ComponentTemplate> template : componentTemplates.entrySet()) {
builder.field(template.getKey(), template.getValue());
builder.field(template.getKey(), template.getValue(), params);
}
builder.endObject();
return builder;
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.startObject();
builder.stringListField(INDEX_PATTERNS.getPreferredName(), this.indexPatterns);
if (this.template != null) {
builder.field(TEMPLATE.getPreferredName(), this.template);
builder.field(TEMPLATE.getPreferredName(), this.template, params);
}
if (this.componentTemplates != null) {
builder.stringListField(COMPOSED_OF.getPreferredName(), this.componentTemplates);
Expand All @@ -216,7 +216,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.field(METADATA.getPreferredName(), metadata);
}
if (this.dataStreamTemplate != null) {
builder.field(DATA_STREAM.getPreferredName(), dataStreamTemplate);
builder.field(DATA_STREAM.getPreferredName(), dataStreamTemplate, params);
}
if (this.allowAutoCreate != null) {
builder.field(ALLOW_AUTO_CREATE.getPreferredName(), allowAutoCreate);
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void writeTo(StreamOutput out) throws IOException {
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(INDEX_TEMPLATE.getPreferredName());
for (Map.Entry<String, ComposableIndexTemplate> template : indexTemplates.entrySet()) {
builder.field(template.getKey(), template.getValue());
builder.field(template.getKey(), template.getValue(), params);
}
builder.endObject();
return builder;
Expand Down
36 changes: 26 additions & 10 deletions server/src/main/java/org/elasticsearch/cluster/metadata/Template.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,25 @@
package org.elasticsearch.cluster.metadata;

import org.elasticsearch.cluster.AbstractDiffable;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.common.xcontent.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ParseField;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.mapper.MapperService;

import java.io.IOException;
import java.util.Base64;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
Expand All @@ -47,8 +48,17 @@ public class Template extends AbstractDiffable<Template> implements ToXContentOb

static {
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> Settings.fromXContent(p), SETTINGS);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) ->
new CompressedXContent(Strings.toString(XContentFactory.jsonBuilder().map(p.mapOrdered()))), MAPPINGS);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> {
Map<String, Object> values = p.mapOrdered();
Object compressed = values.get("compressed");
if (compressed == null) {
return new CompressedXContent(Strings.toString(XContentFactory.jsonBuilder().map(values)));
} else if (compressed instanceof String) {
Copy link
Contributor

@original-brownbear original-brownbear Oct 7, 2021

Choose a reason for hiding this comment

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

No need for this if we do things the same way we do them for mappings.

Copy link
Contributor Author

@probakowski probakowski Oct 12, 2021

Choose a reason for hiding this comment

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

This handles situation when you use binary parameter with JsonXContent - it stores byte arrays as BASE64 encoded strings. Without it ToAndFromJsonMetadataTests#testSimpleJsonFromAndTo fails

return new CompressedXContent(Base64.getDecoder().decode((String) compressed));
} else {
return new CompressedXContent((byte[]) compressed);
}
}, MAPPINGS);
PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> {
Map<String, AliasMetadata> aliasMap = new HashMap<>();
while ((p.nextToken()) != XContentParser.Token.END_OBJECT) {
Expand Down Expand Up @@ -160,11 +170,17 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.endObject();
}
if (this.mappings != null) {
Map<String, Object> uncompressedMapping =
XContentHelper.convertToMap(this.mappings.uncompressed(), true, XContentType.JSON).v2();
if (uncompressedMapping.size() > 0) {
builder.field(MAPPINGS.getPreferredName());
builder.map(reduceMapping(uncompressedMapping));
if (Metadata.CONTEXT_MODE_GATEWAY.equals(params.param(Metadata.CONTEXT_MODE_PARAM, Metadata.CONTEXT_MODE_API))) {
Copy link
Contributor

@original-brownbear original-brownbear Oct 7, 2021

Choose a reason for hiding this comment

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

It would be nice if this followed the exact same conventions we use for mappings, respecting the binary parameter and serializing as bytes for everything but the API.

Copy link
Contributor Author

@probakowski probakowski Oct 12, 2021

Choose a reason for hiding this comment

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

I've added binary parameter and changed usage to similar to IndexMetada

builder.startObject(MAPPINGS.getPreferredName());
builder.field("compressed", mappings.compressed());
builder.endObject();
} else {
Map<String, Object> uncompressedMapping =
XContentHelper.convertToMap(this.mappings.uncompressed(), true, XContentType.JSON).v2();
if (uncompressedMapping.size() > 0) {
builder.field(MAPPINGS.getPreferredName());
builder.map(reduceMapping(uncompressedMapping));
}
}
}
if (this.aliases != null) {
Expand Down
10 changes: 10 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/XContentTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Random;
Expand Down Expand Up @@ -88,6 +89,15 @@ private static String differenceBetweenObjectsIgnoringArrayOrder(String path, Ob
} else {
return path + ": first element is null, the second element is not null";
}
} else if (first instanceof byte[]) {
Copy link
Contributor

@original-brownbear original-brownbear Oct 7, 2021

Choose a reason for hiding this comment

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

Probably also not necessary to extend things this way if we just do what we do for mappings and have a special binary param path here I think.

Copy link
Contributor Author

@probakowski probakowski Oct 12, 2021

Choose a reason for hiding this comment

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

This is required for ESIntegTestCase#ensureClusterStateCanBeReadByNodeTool to pass - this test uses binary serialization and this is the first instance where we have to compare byte[] in it.

Copy link
Contributor

@original-brownbear original-brownbear Oct 13, 2021

Choose a reason for hiding this comment

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

Why don't we have that for mappings already, they should be binary serialized as well and appear to run through the exact same code?

Copy link
Contributor Author

@probakowski probakowski Oct 14, 2021

Choose a reason for hiding this comment

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

There was different path with parsing IndexMetadata->bytes->IndexMetada and ComposableIndexMetadata->bytes->UnknownMetadataCustom when using ElasticsearchNodeCommand#namedXContentRegistry so results differed when using binary serialization. I changed ElasticsearchNodeCommand to parse ComposableIndexMetadata so the code is no longer needed.

if (second instanceof byte[]) {
byte[] bf = (byte[]) first;
byte[] bs = (byte[]) second;
return Arrays.compare(bf, bs) == 0 ? null :
path + ": the elements don't match: " + Arrays.toString(bf) + " != " + Arrays.toString(bs);
} else {
return path + ": second not a byte array (got" + second + ")";
}
} else if (first instanceof List) {
if (second instanceof List) {
List<Object> secondList = new ArrayList<>((List<Object>) second);
Expand Down