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 a template parameter to override auto_create_index value #61858

Merged
merged 78 commits into from
Oct 26, 2020

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Sep 2, 2020

Closes #20640.

This PR introduces a new parameter to v2 templates, allow_auto_create, which allows templates to override the cluster setting auto_create_index. Notes:

  • AutoCreateIndex now looks for a matching v2 template, and if its allow_auto_create setting is true, it overrides the usual logic.
  • TransportBulkAction previously used AutoCreateIndex to check whether missing indices should be created. After talking to @martijnvg, we now rely on AutoCreateAction, which was already differentiating between creating indices and creating data streams. I've updated AutoCreateAction to use AutoCreateIndex.
  • Most of the Java file changes are due to introducing an extra constructor parameter to ComposableIndexTemplate.
  • I've added the new setting to various x-pack templates
  • I added a YAML test to check that watches can be created even when auto_create_index is false.

@pugnascotia pugnascotia marked this pull request as draft September 2, 2020 14:58
@pugnascotia pugnascotia changed the title Add a template parameter to override auto_create_index value [Draft] Add a template parameter to override auto_create_index value Sep 2, 2020
@pugnascotia
Copy link
Contributor Author

@martijnvg draft PR as discussed in Slack.

cc @jaymode for your interest.


// Templates can override the AUTO_CREATE_INDEX_SETTING setting
final ComposableIndexTemplate template = findTemplate(index, state.metadata());
if (template != null && template.getAllowAutoCreate()) {
Copy link
Member

Choose a reason for hiding this comment

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

We talked about the auto create index logic and the allow auto create template flag yesterday. This logic is invoked from the bulk action and is evaluated on the coordinating node. While the decision the auto create an index or data stream is done in the AutoCreateAction on the elected master node.

The reason why we decided to make the decision to auto create an index or data stream in the AutoCreateAction and not here is, because if for some reason a node is lacking behind and doesn't have all the templates then an index is auto created instead of a data stream. Fixing the erroneous situation requires a lot of manual work, hence we moved making this decision to elected master node.

The question is whether the auto create index check (with this enhancement) should be moved to AutoCreateAction class too, so that all the decisions are made on the elected master node.

I'm currently thinking the following:

  • In the case a coordinating node doesn't have the latest action.auto_create_index setting values or composable index templates then the worst that can happen is a new few write requests fail, because the target index doesn't exist.
  • Currently if action.auto_create_index is set to false then also data streams are no longer created. I think this is a bug, because the name of the setting implies auto creation of indices only.
  • If this logic is moved to AutoCreateAction class then the overhead is that remote requests are made to the elected master node in the case target indices don't exist. Most requests will index into an existing data stream or index, so the cost is acceptable.
  • I remember that we eventually want to deprecate and remove the action.auto_create_index setting, but I may be mistaken.

If we are going to remove action.auto_create_index with the new allow auto create template flag then I think let's keep it here? And then maybe also add logic here that if a non existing index matches with a template that auto create data streams then checking the action.auto_create_index setting should be ignored?

@pugnascotia
Copy link
Contributor Author

@martijnvg can you take a look at what I've done here with AutoCreateAction?

@martijnvg
Copy link
Member

@pugnascotia I like the overall direction of this PR. Moving the auto create index check to AutoCreateAction is a good change, since that fixes the problem that data streams are no longer auto created if action.auto_create_index is set to false, at the cost of an additional hop to elected master node (this cost is neglectable, as we only redirect to auto create action is index is missing and in order to create the missing index the extra hop to elected master node needs to be done anyway).

nik9000 and others added 10 commits October 21, 2020 12:10
We have an `@After` annotated method in `AggregatorTestCase` that cleans
up releasibles. But it was `private`! Confusingly, it seemed to be
working! I'm not sure why. This makes it public which is a little more
sensible.
Now that we've got regexes enabled by default (elastic#63029) this adds a test
to runtime fields just to make sure that it works with regexes. It does,
but this adds a test to make sure it continues to work.
This commit extends the set of default metrics for the
data frame analytics evaluation API to all available metrics.
The motivation is that if the user skips setting an explicit
set of metrics, they get most of the evaluation offering.
Run time field that emits geo points from lat/lon values.
Currently when searching with an empty string as lower bound for a range query
on text-based fields we return all documents when 'gte' is used (including the
lower bound) but no documents when 'gt' is used. This might seem
counterintuitive since every value should be greate than the empty string. 
The bug has been fixed in Lucene and this PR adds a test for assuring we observe
the fixed behaviour on searches now.

Closes elastic#63386
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

This LGTM now, thanks for dealing with all the details around the interaction with data streams! I know multiple rounds of back-and-forth like that can be frustrating.

docs/reference/indices/put-component-template.asciidoc Outdated Show resolved Hide resolved
Mention data streams in the docs changes.

Co-authored-by: Gordon Brown <[email protected]>
@pugnascotia
Copy link
Contributor Author

@jaymode are you happy with these changes now? There's still a "changes requested" on the PR.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

@jaymode
Copy link
Member

jaymode commented Oct 22, 2020

There's still a "changes requested" on the PR.

If you don't mind, can you re-request a review once you feel the PR is ready for me to look at the next time.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Left one small comment. LGTM otherwise.

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.Matchers.containsString;

public class AutoCreateIndexIT extends ESRestTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe distribution/archives/integ-test-zip is a better place for this test class? Given that it doesn't extend HttpSmokeTestCase and this test class doesn't seem to require to start a node with special settings or plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure. I tried before to find out where is the "correct" place for REST tests, but couldn't get a clear answer. I think I'll leave this test alone and start an email thread about it. I feel that it ought to be easier to say where a REST test should live.

@pugnascotia pugnascotia merged commit dc855ad into elastic:master Oct 26, 2020
@pugnascotia pugnascotia deleted the 20640-auto-create-templates branch October 26, 2020 12:35
@pugnascotia
Copy link
Contributor Author

This ought to be backported after #63122, I'm trying to find out what is happening with that PR.

pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Oct 27, 2020
…#61858)

Closes elastic#20640.

This PR introduces a new parameter to v2 templates, `allow_auto_create`,
which allows templates to override the cluster setting `auto_create_index`.
Notes:

   * `AutoCreateIndex` now looks for a matching v2 template, and if its
     `allow_auto_create` setting is true, it overrides the usual logic.
   * `TransportBulkAction` previously used `AutoCreateIndex` to check
     whether missing indices should be created. We now rely on
     `AutoCreateAction`, which was already differentiating between creating
     indices and creating data streams.  I've updated `AutoCreateAction` to
     use `AutoCreateIndex`. Data streams are also influenced by
     `allow_auto_create`, in that their default auto-create behaviour can
     be disabled with this setting.
   * Most of the Java file changes are due to introducing an extra
     constructor parameter to `ComposableIndexTemplate`.
   * I've added the new setting to various x-pack templates
   * I added a YAML test to check that watches can be created even when
     `auto_create_index` is `false`.
pugnascotia added a commit that referenced this pull request Nov 5, 2020
Backport of #61858.

Closes #20640.

This PR introduces a new parameter to v2 templates, `allow_auto_create`,
which allows templates to override the cluster setting `auto_create_index`.
Notes:

   * `AutoCreateIndex` now looks for a matching v2 template, and if its
     `allow_auto_create` setting is true, it overrides the usual logic.
   * `TransportBulkAction` previously used `AutoCreateIndex` to check
     whether missing indices should be created. We now rely on
     `AutoCreateAction`, which was already differentiating between creating
     indices and creating data streams.  I've updated `AutoCreateAction` to
     use `AutoCreateIndex`. Data streams are also influenced by
     `allow_auto_create`, in that their default auto-create behaviour can
     be disabled with this setting.
   * Most of the Java file changes are due to introducing an extra
     constructor parameter to `ComposableIndexTemplate`.
   * I've added the new setting to various x-pack templates
   * I added a YAML test to check that watches can be created even when
     `auto_create_index` is `false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates >enhancement Team:Data Management Meta label for data/management team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace action.auto_create_index with a template setting