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 comma-separated feature parsing for GetIndicesAction #24723

Merged
merged 1 commit into from Jun 2, 2017

Conversation

Projects
None yet
5 participants
@dakrone
Copy link
Member

commented May 16, 2017

This removes the parsing of things like GET /idx/_aliases,_mappings, instead,
a user must choose between retriving all index metadata with GET /idx, or only
a specific form such as GET /idx/_settings.

Relates to (and is a prerequisite of) #24437

@dakrone dakrone requested a review from jasontedor May 16, 2017

@dakrone dakrone force-pushed the dakrone:remove-comma-parsing-of-features branch 3 times, most recently May 17, 2017

@dakrone

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@jasontedor could you take a look at this? This is the PR you asked me to separate from #24437

@abeyad abeyad self-requested a review Jun 2, 2017


[source,js]
--------------------------------------------------
GET twitter/_settings,_mappings

This comment has been minimized.

Copy link
@pickypg

pickypg Jun 2, 2017

Member

This could just be changed to

GET twitter?filter_path=*.settings,*.mappings

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

I think we could, but not sure it's worth it since I think this is a very minor edge case that people use.

@abeyad
Copy link
Contributor

left a comment

@dakrone I left some comments

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllAliasesAction.java Outdated
writeAliases(response.aliases().get(index), builder, request);
}
builder.endObject();

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

nit: extra newline

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllMappingsAction.java Outdated
writeMappings(response.mappings().get(index), builder);
}
builder.endObject();

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

nit: extra new line

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllSettingsAction.java Outdated
writeSettings(response.settings().get(index), builder, request, defaults);
}
builder.endObject();

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

nit: extra newline

docs/reference/migration/migrate_6_0/rest.asciidoc Outdated

==== Indices information APIs

Previously it was possible to do `GET /_aliases,_mappings` or `GET

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

/s/do/execute

docs/reference/migration/migrate_6_0/rest.asciidoc Outdated
Previously it was possible to do `GET /_aliases,_mappings` or `GET
/myindex/_settings,_alias` by separating mulitple types of requests with commas
in order to retrieve multiple types of information about one or more indices.
This comma-separation for multiple information retrieval has been removed. `GET

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

/s/multiple information retrieval/retrieving multiple pieces of information

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllMappingsAction.java Outdated
final XContentBuilder builder) throws IOException {
builder.startObject("mappings");
{
if (mappings != null) {

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

Under what circumstances would the mappings for an index be null (as opposed to an empty map)? It seems the default for GetIndexResponse is to always have an empty map for mappings (and aliases and settings) and it would only get assigned to a non-null map.

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

I don't think they ever would be, this is a carryover from when I copied this from the other method. I'll remove it.

- do:
indices.get:
index: test_index
feature: _mapping

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

I'm not clear on why this test was eliminated, because we still support {index}/_mapping by its own without the comma-separated multi values?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

This test was eliminated because it was for the indices.get API, whereas we already have tests for the indices.get_mapping API (in another file), so this was just a duplicate test.

- do:
indices.get:
index: test_blias
feature: _mapping

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

Ditto to the comment above

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

Same about the duplicated test


---
"Get index infos with human settings should return index creation date and version in readable format":

- do:
indices.get:
index: test_index
feature: _settings

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

Ditto to the comment above

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

Same about the duplicated test :)

@@ -4,17 +4,12 @@
"methods":[ "GET" ],
"url":{
"path":"/{index}",
"paths":[ "/{index}", "/{index}/{feature}" ],

This comment has been minimized.

Copy link
@abeyad

abeyad Jun 2, 2017

Contributor

Should we also add rest specs and yaml tests for the new REST endpoints introduced in this PR?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 2, 2017

Author Member

Interestingly enough, we already do have these endpoints in the spec, they just weren't in this file :)

See: https://github.com/dakrone/elasticsearch/blob/remove-comma-parsing-of-features/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json#L7

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

Thanks @abeyad, I pushed some commits addressing your feedback

@abeyad

abeyad approved these changes Jun 2, 2017

Copy link
Contributor

left a comment

LGTM, thanks @dakrone

@dakrone dakrone force-pushed the dakrone:remove-comma-parsing-of-features branch 2 times, most recently Jun 2, 2017

Remove comma-separated feature parsing for GetIndicesAction
This removes the parsing of things like `GET /idx/_aliases,_mappings`, instead,
a user must choose between retriving all index metadata with `GET /idx`, or only
a specific form such as `GET /idx/_settings`.

Relates to (and is a prerequisite of) #24437

@dakrone dakrone force-pushed the dakrone:remove-comma-parsing-of-features branch to a32d1b9 Jun 2, 2017

@dakrone dakrone merged commit a32d1b9 into elastic:master Jun 2, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author has signed the CLA
Details
@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

thank you @abeyad :)

@javanna

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

Are we going to also deprecate this in 5.x?

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

Are we going to also deprecate this in 5.x?

Good idea, I'll open a PR

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 2, 2017

Add deprecation logging for comma-separated feature parsing
This is the deprecation logging and documentation for 5.x related to elastic#24723. It
logs when a user does a request like:

```
GET /index/_alias,_mapping
```

dakrone added a commit that referenced this pull request Jun 3, 2017

dakrone added a commit that referenced this pull request Jun 5, 2017

Add deprecation logging for comma-separated feature parsing (#25040)
* Add deprecation logging for comma-separated feature parsing

This is the deprecation logging and documentation for 5.x related to #24723. It
logs when a user does a request like:

```
GET /index/_alias,_mapping
```

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 7, 2017

Include empty mappings in GET /{index}/_mappings requests
Previously this would output:

```
GET /test-1/_mappings

{ }
```

And after this change:

```
GET /test-1/_mappings

{
  "test-1": {
    "mappings": {}
  }
}
```

To bring parity back to the REST output after elastic#24723.

Relates to elastic#25090

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 8, 2017

Return index name and empty map for /{index}/_alias with no aliases
Previously in elastic#24723 we changed the `_alias` API to not go through the
`RestGetIndicesAction` endpoint, instead creating a `RestGetAliasesAction` that
did the same thing.

This changes the formatting so that it matches the old formatting of the
endpoint, before:

```
GET /test-1/_alias

{ }
```

And after this change:

```
GET /test-1/_alias

{
  "test-1": {
    "aliases": {}
  }
}
```

This is related to elastic#25090

dakrone added a commit that referenced this pull request Jun 8, 2017

Include empty mappings in GET /{index}/_mappings requests (#25118)
Previously this would output:

```
GET /test-1/_mappings

{ }
```

And after this change:

```
GET /test-1/_mappings

{
  "test-1": {
    "mappings": {}
  }
}
```

To bring parity back to the REST output after #24723.

Relates to #25090

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

@dakrone dakrone deleted the dakrone:remove-comma-parsing-of-features branch Dec 13, 2017

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Feb 6, 2018

ywelsch added a commit that referenced this pull request Feb 6, 2018

Remove feature parsing for GetIndicesAction (#28535)
Removes dead code. Follow-up of #24723

ywelsch added a commit that referenced this pull request Feb 6, 2018

Remove feature parsing for GetIndicesAction (#28535)
Removes dead code. Follow-up of #24723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.