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

[Doc] Update "Java API Administration >> Indices Administration" page #31906

Closed
lockwobr opened this issue Jul 9, 2018 · 6 comments
Closed
Assignees
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates >docs General docs changes

Comments

@lockwobr
Copy link

lockwobr commented Jul 9, 2018

This example in the Java API Administration docs does not work correctly and is very miss leading.

Example reads:

client.admin().indices().prepareCreate("twitter")   
        .addMapping("\"tweet\": {\n" +              
                "  \"properties\": {\n" +
                "    \"message\": {\n" +
                "      \"type\": \"text\"\n" +
                "    }\n" +
                "  }\n" +
                "}")
        .get();

This example works, but it does not create the index correctly it seems, the example should read

client.admin.indices().prepareCreate("twitter")
         .addMapping("tweet", "message", "type=text").get();
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@colings86 colings86 added :Data Management/Indices APIs APIs to create and manage indices and templates and removed :Core/Java High Level REST Client labels Jul 10, 2018
@cbuescher
Copy link
Member

it does not create the index correctly it seems

Hi @lockwobr, what seems to be the problem with the created index? I just tried it and it works fine. What version of Elasticsearch are you using?

@lockwobr
Copy link
Author

The problem is with the mapping it creates, does not seem right and if you try a more complicated example it will error out.

Example from the docs creates this mapping:

curl -X GET "localhost:9200/twitter/_mapping/"
{
  "twitter": {
    "mappings": {
      "\"tweet\": {\n  \"properties\": {\n    \"message\": {\n      \"type\": \"text\"\n    }\n  }\n}": {}
    }
  }
}

The I think how the function should be called produces this:

curl -X GET "localhost:9200/twitter/_mapping/"
{
  "twitter": {
    "mappings": {
      "tweet": {
        "properties": {
          "message": {
            "type": "text"
          }
        }
      }
    }
  }
}

What lead me down this path is if you try this it errors out all together, so I started to read the code an noticed that you need to call the function way differently.

client.admin().indices().prepareCreate("twitter").addMapping(
      "\"tweet\": {\n" +
        "  \"properties\": {\n" +
        "    \"location\": {\n" +
        "      \"type\": \"geo_shape\", \n" +
        "      \"tree\": \"quadtree\", \n" +
        "      \"precision\": \"10m\"\n" +
        "    }\n" +
        "  }\n" +
        "}").get();

Errors:

org.elasticsearch.indices.InvalidTypeNameException: mapping type name ["tweet": {
  "properties": {
    "location": {
      "type": "geo_shape",
      "tree": "quadtree",
      "precision": "10m"
    }
  }
}] should not include ',' in it
  at org.elasticsearch.index.mapper.MapperService.validateTypeName(MapperService.java:370)
  at org.elasticsearch.index.mapper.MapperService.internalMerge(MapperService.java:401)
  at org.elasticsearch.index.mapper.MapperService.internalMerge(MapperService.java:353)
  at org.elasticsearch.index.mapper.MapperService.merge(MapperService.java:277)
  at org.elasticsearch.cluster.metadata.MetaDataCreateIndexService$IndexCreationTask.execute(MetaDataCreateIndexService.java:444)
  at org.elasticsearch.cluster.ClusterStateUpdateTask.execute(ClusterStateUpdateTask.java:45)
  at org.elasticsearch.cluster.service.MasterService.executeTasks(MasterService.java:643)
  at org.elasticsearch.cluster.service.MasterService.calculateTaskOutputs(MasterService.java:273)
  at org.elasticsearch.cluster.service.MasterService.runTasks(MasterService.java:198)
  at org.elasticsearch.cluster.service.MasterService$Batcher.run(MasterService.java:133)
  at org.elasticsearch.cluster.service.TaskBatcher.runIfNotProcessed(TaskBatcher.java:150)
  at org.elasticsearch.cluster.service.TaskBatcher$BatchedTask.run(TaskBatcher.java:188)
  at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:573)
  at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:244)
  at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:207)
  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  at java.lang.Thread.run(Thread.java:748)

After looking at the code figured out if you change the above to it works.

client.admin.indices().prepareCreate("twitter").addMapping("tweet", "location", "type=geo_shape,tree=quadtree,precision=10m").get()

Version

"version": {
"number": "6.2.2",
"build_hash": "10b1edd",
"build_date": "2018-02-16T19:01:30.685723Z",
"build_snapshot": false,
"lucene_version": "7.2.1",
"minimum_wire_compatibility_version": "5.6.0",
"minimum_index_compatibility_version": "5.0.0"
}

@cbuescher
Copy link
Member

@lockwobr wow, great catch. I stand corrected. Thanks for letting us know, this is certainly an error in the documentation in this case and should be corrected.

@cbuescher cbuescher self-assigned this Jul 10, 2018
@lockwobr
Copy link
Author

lockwobr commented Jul 10, 2018

Your welcome. I kind of think it should be a bug in the client code too, because I feel it shouldn't really let you call it the way it is in the document. There is a check for it in the code, but its not working it seems. It wants pairs of params, and some how it is if you have one it seems to let pass the check.

cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Jul 11, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes elastic#31906
@cbuescher
Copy link
Member

client code too, because I feel it shouldn't really let you call it the way it is in the document

I was very much in favour of restricting the client call and make addMapping("someString") without further arguments illegal. However when trying to do so several of our internal tests failed. Also the current Rest API allowes you to put a type without any mappings, sth. like

PUT test
{
    "mappings" : {
        "type1" : {
        }
    }
}

which is about the same as doing the problematic "addMapping()" call. Since we allow this in REST, we should not really prevent it in the Java API either, although it might not make much sense. I will probably open an issue to discuss whether there is any real reason for having empty mappings. Maybe we want to dissallow this in Rest, but that would have to happen first and needs to be in a mayor version since it would be a breaking change.

cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
cbuescher pushed a commit that referenced this issue Jul 17, 2018
The current docs of the put-mapping Java API is currently broken. It its current
form, it creates an index and uses the whole mapping definition given as a JSON
string as the type name. Since we didn't check the index created in the
IndicesDocumentationIT so far this went unnoticed.

This change adds test to catch this error to the documentation test, changes the
documentation so it works correctly now and adds an input validation to
PutMappingRequest#buildFromSimplifiedDef() which was used internally to reject
calls where no mapping definition is given.

Closes #31906
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 >docs General docs changes
Projects
None yet
Development

No branches or pull requests

4 participants