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

Update mappings for kibana index #9280

Merged
merged 7 commits into from Dec 2, 2016

Conversation

ycombinator
Copy link
Contributor

Fixes #9276.

In elastic/elasticsearch#21670 backwards compatibility of mappings was removed. This included support for the string/not_analyzed mapping; the keyword mapping must be used instead. This PR makes this change in the mapping of the Kibana index (default: .kibana).

Note that this PR fixes the easy part of the problem :) The harder problem is to automatically, at runtime, upgrade the mapping for existing (i.e. pre-6.0) users of Kibana. This upgrade issue will be dealt with in #6409.

@ycombinator
Copy link
Contributor Author

@lukasolson @LeeDr @tylersmalley this is ready for review now.

@tylersmalley
Copy link
Contributor

tylersmalley commented Nov 30, 2016

@ycombinator, looks like there is a failing test: expected 'keyword' to equal 'string'

@@ -38,14 +38,14 @@ export default function IndexPatternFactory(Private, Notifier, config, kbnIndex,
});

const mapping = mappingSetup.expandShorthand({
title: 'string',
timeFieldName: 'string',
title: 'keyword',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want these to be not-analyzed? We might want to use text in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I could tell the title here is the index pattern, for example, logstash-*. So I'm fairly certain this ought to be not analyzed.

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 ended up changing this to text anyway. I explained the reason in f654def.

@ycombinator
Copy link
Contributor Author

BTW, I'm getting this error now when I load up the index patterns page:

Error: [illegal_argument_exception] mapper [title] cannot be changed from type [keyword] to [text]

So there's more work to be done on this PR. Removing review label for now...

@ycombinator
Copy link
Contributor Author

Strange, I don't see the error from my previous comment any more. Perhaps I was switching between branches or something. Marking this as ready for review again.

@lukasolson
Copy link
Member

So, this seems to fix the problems with opening Kibana. But as soon as I choose an index pattern and try to go to discover, I get this error:

Error: [mapper_parsing_exception] No handler for type [string] declared on field [title]
    at respond (elasticsearch.angular.js:23653)
    at checkRespForFailure (elasticsearch.angular.js:23612)
    at elasticsearch.angular.js:299
    at processQueue (angular.js:14745)
    at angular.js:14761
    at Scope.$eval (angular.js:15989)
    at Scope.$digest (angular.js:15800)
    at Scope.$apply (angular.js:16097)
    at done (angular.js:10546)
    at completeRequest (angular.js:10744)

Here's the request causing the error:

PUT https://localhost:5601/mkn/elasticsearch/.kibana/_mapping/search
{
  "search": {
    "properties": {
      "title": {
        "type": "string"
      },
      "description": {
        "type": "string"
      },
      "hits": {
        "type": "integer"
      },
      "columns": {
        "type": "string"
      },
      "sort": {
        "type": "string"
      },
      "version": {
        "type": "integer"
      },
      "kibanaSavedObjectMeta": {
        "properties": {
          "searchSourceJSON": {
            "type": "string"
          }
        }
      }
    }
  }
}

Here's the response:

{
  "error": {
    "root_cause": [
      {
        "type": "mapper_parsing_exception",
        "reason": "No handler for type [string] declared on field [title]"
      }
    ],
    "type": "mapper_parsing_exception",
    "reason": "No handler for type [string] declared on field [title]"
  },
  "status": 400
}

@lukasolson
Copy link
Member

@ycombinator
Copy link
Contributor Author

@Bargs I could you use your help with the first two files listed in #9280 (comment):

The first file, init_default_field_props.js, doesn't seem to be referenced anywhere in the code. So I can't tell how it is being used and what impact any changes to it might have. Do you have any idea if it safe to just delete this file as it doesn't seem to be referenced from anywhere in the codebase?

The second file mapping_overrides.js only seems to be referenced from init_default_field_props.js (the first file). So would it be safe to delete this one as well as it is unreachable from any other part of the codebase?

@ycombinator
Copy link
Contributor Author

@BigFunger Do you know if the string ingest type on this line needs to be changed to text or keyword? My guess is that it should be changed to text but I don't know enough about ingest types to be sure.

This needs to be text because other types in the Kibana index also have a field named "title", whose mapping needs to be text. We can't have the same field have different mappings in different types within the same index.
@Bargs
Copy link
Contributor

Bargs commented Dec 1, 2016

@ycombinator both of those files can be deleted, they're leftovers from the add data stuff.

@ycombinator
Copy link
Contributor Author

I ❤️ you @Bargs

@ycombinator
Copy link
Contributor Author

Checked with @BigFunger about #9280 (comment). That file (src/core_plugins/kibana/server/lib/processors/convert/kibana_to_es_converter.js) isn't needed any more, along with a bunch of other files related to pipelines. He's removing them in #9321. Once that is done (and I've rebased this PR), it will be ready for review again.

@ycombinator ycombinator removed the review label Dec 2, 2016
@ycombinator
Copy link
Contributor Author

Chatted with @BigFunger about the code that's being deleted in #9321. That code is currently not reachable so we don't need to wait for it to be deleted in order to make progress on this PR.

@lukasolson @tylersmalley Mind giving this another looksie?

@tylersmalley
Copy link
Contributor

Looks like it's referenced from the tests.

Mocha Failure: Warning: Cannot find module '../init_default_field_props'� Use --force to continue.

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Things seem to be functional and the changes LGTM.

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM - also went through some smoke testing, especially around saved objects.

@harobed
Copy link

harobed commented Apr 4, 2017

It is possible to backport this patch to Kibana 5.x ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Operations Team label for Operations Team v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

must switch kibana index from string to keyword
7 participants