Skip to content

Conversation

@nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 15, 2022

I broke shard splitting when _routing is required and you use nested
docs. The mapping would look like this:

"mappings": {
  "_routing": {
    "required": true
  },
  "properties": {
    "n": { "type": "nested" }
  }
}

If you attempt to split an index with a mapping like this it'll blow up
with an exception like this:

Caused by: [idx] org.elasticsearch.action.RoutingMissingException: routing is required for [idx]/[0]
	at org.elasticsearch.cluster.routing.IndexRouting$IdAndRoutingOnly.checkRoutingRequired(IndexRouting.java:181)
	at org.elasticsearch.cluster.routing.IndexRouting$IdAndRoutingOnly.getShard(IndexRouting.java:175)

This fixes the problem by entirely avoiding the branch of code. That
branch was trying to find any top level documents that don't have a
_routing. But we know that there aren't any top level documents
without a routing in this case - the routing is "required". ES wouldn't
have let you index any top level documents without the routing.

This also adds a small pile of REST layer tests for shard splitting that
hit various branches in this area. For extra paranoia.

Closes #88109

I broke shard splitting when `_routing` is required and you use `nested`
docs. The mapping would look like this:
```
"mappings": {
  "_routing": {
    "required": true
  },
  "properties": {
    "n": { "type": "nested" }
  }
}
```

If you attempt to split an index with a mapping like this it'll blow up
with an exception like this:
```
Caused by: [idx] org.elasticsearch.action.RoutingMissingException: routing is required for [idx]/[0]
	at org.elasticsearch.cluster.routing.IndexRouting$IdAndRoutingOnly.checkRoutingRequired(IndexRouting.java:181)
	at org.elasticsearch.cluster.routing.IndexRouting$IdAndRoutingOnly.getShard(IndexRouting.java:175)
```

This fixes the problem by entirely avoiding the branch of code. That
branch was trying to find any top level documents that don't have a
`_routing`. But we *know* that there aren't any top level documents
without a routing in this case - the routing is "required". ES wouldn't
have let you index any top level documents without the routing.

This also adds a small pile of REST layer tests for shard splitting that
hit various branches in this area. For extra paranoia.

Closes elastic#88109
@nik9000 nik9000 added >bug :Data Management/Indices APIs APIs to create and manage indices and templates v8.5.0 labels Aug 15, 2022
@nik9000 nik9000 requested review from masseyke and romseygeek August 15, 2022 19:53
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Aug 15, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @nik9000, I've created a changelog YAML for you.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

Tricksy tricksy. LGTM!

@nik9000
Copy link
Member Author

nik9000 commented Aug 16, 2022

Tricksy tricksy. LGTM!

Thanks. It was indeed tricky!

@nik9000 nik9000 merged commit b327b17 into elastic:main Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Data Management/Indices APIs APIs to create and manage indices and templates Team:Data Management Meta label for data/management team v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] SplitIndexIT testSplitFromOneToN failing

3 participants