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.

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

Jump to bottom

Auto generate index.routing_path from mapping #86790

Merged
merged 11 commits into from
May 20, 2022
Merged

Auto generate index.routing_path from mapping #86790

merged 11 commits into from
May 20, 2022

Conversation

martijnvg
Copy link
Member

@martijnvg martijnvg commented May 15, 2022 •
edited
Loading

If no index.routing_path has been specified in the matching template or the component template that the template refers to then attempt to generate the index.routing_path index setting from the matching template's mapping or component template the template refers to.

Relates #74660

@martijnvg martijnvg force-pushed the generate_index_routing_setting branch 2 times, most recently from 11154a5 to 27c3ca8 Compare May 16, 2022 08:32
@martijnvg martijnvg force-pushed the generate_index_routing_setting branch from 27c3ca8 to 64fcfb8 Compare May 16, 2022 08:36
nik9000
nik9000 reviewed May 16, 2022
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

It makes sense to me. I'm not particularly worried about the overhead involved when validating templates. I think it's a fairly infrequent action - just on template PUT. Is that true?

...ata-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java Outdated
@@ -92,4 +118,54 @@ public Settings getAdditionalIndexSettings(
return Settings.EMPTY;
}

// Find fields in mapping that are of type keyword and time_series_dimension enabled.
Copy link
Member

Choose a reason for hiding this comment

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

I'd use javadoc for this. It'll show up when you mouseover the method in your IDE which is kind of nice.

server/src/main/java/org/elasticsearch/node/Node.java Outdated
@@ -633,10 +634,11 @@ protected Node(
searchModule.getRequestCacheKeyDifferentiator()
);

final var parameters = new IndexSettingProvider.Parameters(indicesService::createIndexMapperService);
Copy link
Member

Choose a reason for hiding this comment

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

While I'm thinking about it - could you make another PR that renames createIndexMapperService to createIndexMapperServiceForValidation or something? The name of the method feels so innocuous but it does a genuinely funny thing.

@martijnvg
Copy link
Member Author

I think it's a fairly infrequent action - just on template PUT. Is that true?

This will be used when creating a template (as part of validating), when simulating a template via simulate api and creation of a backing index of data stream. The latter will be the most frequent compared to the other two use cases and happens as part of creating a new data stream or rolling over a data stream. I still think that this is okay.

@martijnvg martijnvg added the :Data Management/Data streams Data streams and their lifecycles label May 17, 2022
@martijnvg martijnvg mentioned this pull request May 17, 2022
Closed
@martijnvg martijnvg marked this pull request as ready for review May 17, 2022 08:24
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label May 17, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@martijnvg
Copy link
Member Author

Pinging @jsoriano for awareness.

@martijnvg martijnvg requested a review from nik9000 May 17, 2022 14:01
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 17, 2022
@martijnvg
Rename IndicesService#createIndexMapperService() method
66d20ce 
Rename from `createIndexMapperService(...)`` to `createIndexMapperServiceForValidation(...)`

Followup from elastic#86790
@martijnvg martijnvg mentioned this pull request May 17, 2022
Merged
martijnvg added a commit that referenced this pull request May 17, 2022
@martijnvg
Rename IndicesService#createIndexMapperService() method (#86851)
4023012 
Rename from `createIndexMapperService(...)`` to `createIndexMapperServiceForValidation(...)`

Followup from #86790
@jsoriano
Copy link
Member

Thanks!

@martijnvg martijnvg merged commit 6bd6fbd into elastic:master May 20, 2022
@jsoriano jsoriano mentioned this pull request Jun 21, 2022
Closed
@martijnvg martijnvg mentioned this pull request Sep 29, 2022
Closed
This was referenced Nov 9, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Data Management/Data streams Data streams and their lifecycles >non-issue Team:Data Management Meta label for data/management team v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martijnvg @elasticmachine @jsoriano @nik9000 @elasticsearchmachine