-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Allows multiple patterns to be specified for index templates #21009
Allows multiple patterns to be specified for index templates #21009
Conversation
@a2lin, Sorry to let this sit for a few days. I've added this to my work queue (browser tabs...) and I promise I'll get to it before too long. My queue is getting longer than I'd like these days. |
if(entry.getValue() instanceof String) { | ||
template(Collections.singletonList((String) entry.getValue())); | ||
} else if (entry.getValue() instanceof List) { | ||
template((List<String>) entry.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth doing something like ((List<?>) entry.getValue()).stream().map(Object::toString).collect(toList())
so we are sure we have strings here. As you've written it now if someone sticks [0]
in the template
field then we'll cram an Integer
in the list and we'll get ClassCastException
s in weird places.
} else if (entry.getValue() instanceof List) { | ||
template((List<String>) entry.getValue()); | ||
} else { | ||
throw new IllegalArgumentException("Malformed [template] value, should be a string[]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "should be a string or a list of strings".
@@ -449,7 +458,7 @@ public void readFrom(StreamInput in) throws IOException { | |||
super.readFrom(in); | |||
cause = in.readString(); | |||
name = in.readString(); | |||
template = in.readString(); | |||
template = in.readList(StreamInput::readString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! New fun things for you to learn about. The way we support mixed version cluster (like, during a rolling restart) is by adding branches to this code. So this needs to read like:
if (in.getVersion().onOrAfter(Version.5_1_0)) {
template = in.readList(StreamInput::readString);
} else {
template = singletonList(in.readString());
}
And the out
needs to have the same branching. And you need to find some place to cram the test for this.
The specific version number that I chose is based on my expectation of which release will be the first to see this feature.
@@ -286,7 +289,13 @@ public PutIndexTemplateRequest source(Map templateSource) { | |||
for (Map.Entry<String, Object> entry : source.entrySet()) { | |||
String name = entry.getKey(); | |||
if (name.equals("template")) { | |||
template(entry.getValue().toString()); | |||
if(entry.getValue() instanceof String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good that you've added single string support - this is needed so we can add this feature without breaking backwards compatibility. We could chose to delay this to 6.0 and only support lists here but I don't think that is a good idea anyway.
@@ -41,7 +42,7 @@ public PutIndexTemplateRequestBuilder(ElasticsearchClient client, PutIndexTempla | |||
/** | |||
* Sets the template match expression that will be used to match on indices created. | |||
*/ | |||
public PutIndexTemplateRequestBuilder setTemplate(String template) { | |||
public PutIndexTemplateRequestBuilder setTemplate(List<String> template) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add a new method that takes a String does a singletoneList
on the input for backwards compatibility but we don't tend to do that for these builders because we expect that users of the Java API can deal with these changes on upgrade because the compiler will catch it for them. In other words: we don't respect semver in the Java API.
@@ -85,7 +87,7 @@ private Table buildTable(RestRequest request, ClusterStateResponse clusterStateR | |||
if (patternString == null || Regex.simpleMatch(patternString, indexData.name())) { | |||
table.startRow(); | |||
table.addCell(indexData.name()); | |||
table.addCell(indexData.getTemplate()); | |||
table.addCell(String.format(Locale.ROOT, "[%s]", String.join(", ", indexData.getTemplate()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just do "[" + String.join(", ", indexData.getTemplate()) + "]"
. I was a big fan of String.format
for a while but I've fallen out of love. And the compiler is quite good at optimizing +
for Strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really with it were indexData.getTemplate*s*()
or indexData.getPatterns()
though.
@@ -383,7 +384,7 @@ public void doAfterNodes(int numNodes, Client client) throws Exception { | |||
|
|||
ClusterState state = client().admin().cluster().prepareState().execute().actionGet().getState(); | |||
assertThat(state.metaData().index("test").mapping("type2"), notNullValue()); | |||
assertThat(state.metaData().templates().get("template_1").template(), equalTo("te*")); | |||
assertThat(state.metaData().templates().get("template_1").template(), equalTo(Collections.singletonList("te*"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I do a static import for singletonList
but I know lots of folks don't. If you want to keep it as is that is fine with me, or if you want to do static import that is fine with me.
@@ -3,8 +3,8 @@ | |||
|
|||
Index templates allow you to define templates that will automatically be | |||
applied when new indices are created. The templates include both settings and | |||
mappings, and a simple pattern template that controls whether the template | |||
should be applied to the new index. | |||
mappings, and a list of simple pattern templates that controls whether the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a simple pattern or a list of simple patterns?
@@ -15,7 +15,7 @@ For example: | |||
-------------------------------------------------- | |||
PUT _template/template_1 | |||
{ | |||
"template": "te*", | |||
"template": ["te*"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might make this one ["te*", "bar*"]
but then leave the others as strings rather than lists.
@@ -64,7 +64,7 @@ | |||
- match: | |||
$body: / | |||
(^|\n)test_2 \s+ | |||
test-2\* \s+ | |||
\[test-2\*\] \s+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the \s+
so it lines up with the \s+
above it?
88ac860
to
24b4bf5
Compare
@nik9000 Sorry for the delayed response! I think I fixed all the points. There's a bit of oddness left, but I can fix #18922 (rename template to index_pattern in the rest api) after this one and that should change everything. I've added the For the in/out tests I used:
and a similar variant for the PutIndexTemplateRequest to generate the base64 strings used as constants. Thanks again for reviewing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few very minor things. I think this is just about ready! Thanks for being so patient with me. I keep getting distracted and forgetting about this. Sorry!
Anyway, I mention rebasing in one of my minor points. If we didn't have any remaining issues I'd say to just squash and rebase and fix those points. But since we have some open stuff I'd wait on those points until later.
} | ||
} else if (name.equals("order")) { | ||
order(XContentMapValues.nodeIntegerValue(entry.getValue(), order())); | ||
} else if ("version".equals(name)) { | ||
if ((entry.getValue() instanceof Integer) == false) { | ||
if (!(entry.getValue() instanceof Integer)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually prefer == false
or false ==
of !
. Well, we have one core contributor who really prefers it and the rest of us are fine with it. false ==
is unlikely to be misread, whereas !
is small and can get lost if you scan the code quickly. So I'd change this back. It is weird, yes, but it is our kind of weird.
@@ -47,6 +47,8 @@ | |||
|
|||
public class IndexTemplateMetaData extends AbstractDiffable<IndexTemplateMetaData> { | |||
|
|||
public static final Version V_5_1_0 = Version.fromId(5010099); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
BytesArray bytes = new BytesArray(Base64.getDecoder().decode(putRequestBytes)); | ||
|
||
try (StreamInput in = bytes.streamInput()) { | ||
in.setVersion(Version.V_5_0_0_rc1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you rebase you can use V_5_0_0
which is more correct, I think. Both are fine, but we really expect wire compatibility with 5.0.0, not 5.0.0-rc1.
BytesArray bytes = new BytesArray(Base64.getDecoder().decode(templateBytes)); | ||
|
||
try (StreamInput in = bytes.streamInput()) { | ||
in.setVersion(Version.V_5_0_0_rc1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about the version constant here.
@@ -429,7 +429,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |||
IndexTemplateMetaData templateMetaData = cursor.value; | |||
builder.startObject(templateMetaData.name()); | |||
|
|||
builder.field("template", templateMetaData.template()); | |||
builder.field("template", templateMetaData.patterns()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I think we should make this index_patterns
or patterns
. We're already making a breaking change here (string -> list) so let's make the output more readable while we are here. I'm unsure if this means we can't merge this to the 5.x branch or not.... I hadn't thought this through until now. It is is a fairly out of the way section of the code....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all the docs I could find on this API. So it is fairly vague about the return format.
@nik9000 I've addressed the bit about the boolean and also the ClusterState. Thanks again for the reviews! I'll rebase on top of master and fix the versions (& the VersionTests thing) if these changes seem okay. |
de12ba5
to
77df805
Compare
I think you are already breaking there because you change from a string to a list. Honestly I think you should just change it. Either we say "this has to go into 6.0 because of this" or we say "this part of the API is so deep we're willing to break it in 5.1". I don't think you can get away with not switching to a list though. If you are willing to make the We should also add a note to the breaking changes docs about the |
77df805
to
962dbb6
Compare
@nik9000 Okay, I rebased and fixed things! (This change solves #18922 as well now since the metadata causes A summary of the changes: For the
and also with
giving the same Directly specifying the parameters for Support for Performing a
The cat_templates api returns:
and for multiple patterns:
As for the Java PutIndexRequestBuilder API: Going to and from pre-(this patch): If you do a backwards roll from 5.1.0 back to 5.0.0, all of the template patterns will be seen as null (as they have been rewritten in the metadata to use the Sorry for the essay, and thanks again for all the reviews and comments! |
962dbb6
to
512da39
Compare
index template, and rename the field from 'template' to 'index_pattern'. Closes elastic#20690
512da39
to
ca18747
Compare
Hi @a2lin Thanks for all of the effort you've put into this. It'd be good to have some deprecation logging when the user specifies |
See |
So so so so much stuff is likely to break if you try to downgrade. On the other hand, in a mixed version cluster you might get this behavior you describe if you query a node on the old version. |
@clintongormley what is your opinion on the target branch for this? I'd love the change to go into 5.x but it breaks backwards compatibility on the return for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two super minor things and @clintongormley left one a request to add deprecation logging when you use the old parameter.
@@ -38,11 +40,16 @@ public PutIndexTemplateRequestBuilder(ElasticsearchClient client, PutIndexTempla | |||
super(client, action, new PutIndexTemplateRequest(name)); | |||
} | |||
|
|||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you copy the javadoc on the method below to this method as well? Including the @Deprecate
javadoc annotation telling folks to use #setPatterns
instead?
} | ||
} else if (token.isValue()) { | ||
// This is for roll-forward bwc with (#21009) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change the comment to something like "prior to 5.1 Elasticsearch only supported a single index pattern and called it template
."
@nik9000 I've completed the changes. I found it easier to just use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have some failing documentation tests. You should be able to reproduce with gradle docs:check
. Can you fix them?
Debugging docs tests can be a bit of a trip. Have a look in |
I think that the docs failures were caused by the deprecation logging. Deprecation logging causes Anyway, I think you can fix these failures by using the new name. |
@nik9000 Sorry about that :(. |
No problem! It is sneaky. I'm retesting one last time locally to be super ultra paranoid but everything is looking good so far. |
Merged! Thanks again @a2lin! |
I still owe a breaking changes doc patch for this. I'll write one in the morning. |
Thanks for all the help and reviews! |
Breaking changes docs: 7c38867 |
* master: ShardActiveResponseHandler shouldn't hold to an entire cluster state Ensures cleanup of temporary index-* generational blobs during snapshotting (#21469) Remove (again) test uses of onModule (#21414) [TEST] Add assertBusy when checking for pending operation counter after tests Revert "Add trace logging when aquiring and releasing operation locks for replication requests" Allows multiple patterns to be specified for index templates (#21009) [TEST] fixes rebalance single shard check as it isn't guaranteed that a rebalance makes sense and the method only tests if rebalance is allowed Document _reindex with random_score
Sorry for asking (and not changing this myself via pull request), it would be great to add a statement to the docs just what that "pattern" can be. I was hoping for a "regexp", but it looks like it is in fact a "glob". Most examples out there suggest it might be a "suffix asterisk" only, Thanks. |
@@ -286,7 +296,20 @@ public PutIndexTemplateRequest source(Map templateSource) { | |||
for (Map.Entry<String, Object> entry : source.entrySet()) { | |||
String name = entry.getKey(); | |||
if (name.equals("template")) { | |||
template(entry.getValue().toString()); | |||
// This is needed to allow for bwc (beats, logstash) with pre-5.0 templates (#21009) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nik9000 I assume you mean here pre-6.0 templates. Or will there be support for indext-patterns
in the 5.x releases?
The change was introduced in elastic/elasticsearch#21009 and deprecates old `template` field. This change uses the new `index_patterns` field and keeps compatibility with `template` for older version
The change was introduced in elastic/elasticsearch#21009 and deprecates old `template` field. This change uses the new `index_patterns` field and keeps compatibility with `template` for older version
The `template` field was deprecated in 6.0 in favor of `index_patterns`, and can now be removed. Relates to #21009.
The `template` field was deprecated in 6.0 in favor of `index_patterns`, and can now be removed. Relates to elastic#21009.
Index templates now take an list for the template: field instead of a string.
For example:
Instead of the previous form
Template metadata from older versions of ES are still openable with this code, although I don't know how important that is since it's probably assumed that you can't just open a new elasticsearch release on an old one's data directory and expect it to work.
Closes #20690