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

Remove mapping type usage #2979

Closed
5 tasks
dreamer-89 opened this issue Apr 19, 2022 · 7 comments
Closed
5 tasks

Remove mapping type usage #2979

dreamer-89 opened this issue Apr 19, 2022 · 7 comments
Assignees
Labels
enhancement Enhancement or improvement to existing feature or request Indexing & Search v2.1.0 Issues and PRs related to version 2.1.0

Comments

@dreamer-89
Copy link
Member

dreamer-89 commented Apr 19, 2022

Is your feature request related to a problem? Please describe.

As part of #1940 we completed all breaking changes as part of 2.0 release. In this issue, we plan to remove all remaining type usages from OpenSearch engine internals.

Background

The mapping types have been deprecated in Elasticsearch (starting from 7.0) [1] with breaking changes to the index creation, put mapping, get mapping, put template, get template and get field mappings APIs and on track to be removed in 8.0 [2]. This is the debt Opensearch has inherited and the code related to mapping types support has to be removed.
[1] https://www.elastic.co/guide/en/elasticsearch/reference/current/removal-of-types.html
[2] elastic/elasticsearch#41059

Additional context

[Removal of mapping types] (https://www.elastic.co/guide/en/elasticsearch/reference/current/removal-of-types.html)
Captures remaining pending changes for Type mapping removal

Related: #1940

Pending changes

@dreamer-89 dreamer-89 added enhancement Enhancement or improvement to existing feature or request untriaged labels Apr 19, 2022
@dblock dblock changed the title Remove mapping types Remove mapping type usage Apr 19, 2022
@dblock dblock added the v2.1.0 Issues and PRs related to version 2.1.0 label Apr 19, 2022
@elfisher
Copy link

Are we tracking this as part of a campaign to remove mapping types usage?

@dreamer-89
Copy link
Member Author

Are we tracking this as part of a campaign to remove mapping types usage?

This tracks the pending internal and non-breaking remaining changes around removing mapping types.

@dreamer-89
Copy link
Member Author

@reta @nknize : I listed down remaining _type references in the code base. Need your input on below.

  1. Run bulk index benchmark takes user input for typeName. Code usage, defined here. type is taken as input from CLI. Is this benchmark still used today ?
  2. BulkRequestParser.java has elseif check on _type field. This check should be removed as _type should be addressed as any other random word in request body; which is already handled here. See this added previously and referred in BulkRequestParserTests inside testTypesStillParsedForBulkMonitoring.
  3. Can we remove type validation from MapperService & clean up tests. In 2.0 no type field is accepted and older indices would have already been validated.

Identified other references which can be removed without any issue.

  1. ScriptProcessor, ConditionalProcessor. Is it possible to provide _type from curl request and get these deprecations ?
  2. Multiple references of _type in _ingest/pipeline/_simulate API across multiple places. Verified that _type is ignored by engine. Reference in a. IngestRequestConverterTests.java b. IngestClientIT.java in testSimulatePipeline c. grok_120.yml yaml tests
  3. ForEachProcessorTests.java need to remove fieldValue in TestProcessor constructor.
  4. 60_deprecated.yml verifying deprecated type in bulk action.
  5. CustomScriptPlugin adds _type field. Probably ignored on engine side and thus can be removed
  6. MultiTermVectorsResponse.java has _type defined in Fields class. It is not referenced and can be removed.
  7. UpdateHelper.java defines _type in ContextFields but not referenced anywhere.
  8. Bunch of _type references in mocked API responses of reindex module modules/reindex/src/test/resources/responses/*.json
  9. Tests for tests ClientYamlTestSuitesTests, DoSectionTests, OpenSearchClientYamlSuiteTestCaseTests have hard-coded request body.
  10. HLRC TermsVectorRequest still uses _type in toXContent.

@reta
Copy link
Collaborator

reta commented Apr 25, 2022

Thanks for summarizing, @dreamer-89, regarding your questions

  1. Run bulk index benchmark takes user input for typeName. Code usage, defined here. type is taken as input from CLI. Is this benchmark still used today ?

I actually don't know to be honest, but cleaning it up would be good in any case.

  1. Can we remove type validation from MapperService & clean up tests.

It looks safe to be removed, 1.x does validate mapping types, so 2.0 should not run into that, @nknize wdyt?

@dreamer-89
Copy link
Member Author

Thanks for summarizing, @dreamer-89, regarding your questions

  1. Run bulk index benchmark takes user input for typeName. Code usage, defined here. type is taken as input from CLI. Is this benchmark still used today ?

I actually don't know to be honest, but cleaning it up would be good in any case.

Thanks @reta. Created issue #3074 to track this.

@dreamer-89
Copy link
Member Author

dreamer-89 commented Apr 26, 2022

This plugin seems to be not updated since 6.x as it uses custom typed end-point /geonames/{type}/_noop_bulk for ingestion. This type param (taken from CLI) is used only in action meta data; while hard-coded type is used in rest end-point (/geonames/type/_noop_bulk); which means for all values of this param except type; this tool was broken. With removal of type param; this PR also fixes this issue.

We should revisit if this client-benchmark-noop-api-plugin is still used. Opensearch already has benchmark tools which can probably replace this but not sure if that needs extra work to setup this plugin.

CC: @saratvemulapalli @VachaShah

@dreamer-89
Copy link
Member Author

Closing this one as most of the changes have been completed except type validation removal from MapperService which needs more discussion (pointed below). Will open a separate issue to track that (and other type related usages if any). Closing this one as remaining changes have been merged.

Can we remove type validation from MapperService & clean up tests. In 2.0 no type field is accepted and older indices would have already been validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Indexing & Search v2.1.0 Issues and PRs related to version 2.1.0
Projects
None yet
Development

No branches or pull requests

5 participants