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

Added GET Index API #7234

Merged
merged 1 commit into from Sep 11, 2014
Merged

Added GET Index API #7234

merged 1 commit into from Sep 11, 2014

Conversation

colings86
Copy link
Contributor

Returns information about settings, aliases, warmers, and mappings. Basically returns the IndexMetadata.

Closes #4069

@colings86
Copy link
Contributor Author

@jpountz Comment from @clintongormley was that it would be good to move the logic for the /{index}/_warmers|_mappings|_settings|_alaises/{name} etc into the same action. I think this is a good idea but I am wondering if it should be done in this change or in another ticket?

@jpountz
Copy link
Contributor

jpountz commented Aug 12, 2014

The idea sounds good to me as well but this code is not the one I know best, maybe @javanna can comment?


[source,js]
--------------------------------------------------
$ curl -XGET 'http://localhost:9200/twitter/_settings,_mapping'
Copy link
Member

Choose a reason for hiding this comment

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

not too sure about the _ prefix, also mapping might need to be plural, @clintongormley can you have a look as well please?

Choose a reason for hiding this comment

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

Grrrr singular vs plural again... Elsewhere we have _mapping, _warmer, _alias/_aliases, _settings

I think, for ease of use, we should support singular and plural varieties of everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually both are support but i was only going to document the plural form since its the one that makes most sense here. This instance of the singular was a typo

@javanna
Copy link
Member

javanna commented Aug 12, 2014

Looks good, I left some comments and summoned @clintongormley to have a look api-wise :) I think we should isolate the breaking changes (removal of REST endpoints in favour of the new ones) to another PR and discuss them there.

@colings86
Copy link
Contributor Author

@javanna implemented the changes you suggested and left a few comment too

"type":"boolean",
"description":"Whether aliases pointing to multiple indices are allowed (default: false)"
},
"forbid_closed_indices":{
Copy link
Member

Choose a reason for hiding this comment

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

I think you exposed too many options :) these two are not exposed to the REST layer, are kinda hidden options that we use internally... look here: https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/action/support/IndicesOptions.java#L147

@javanna javanna removed the review label Aug 13, 2014
@javanna
Copy link
Member

javanna commented Aug 13, 2014

Done another review round, the main thought is that this should become a proper java api given that the goal is to replace get settings and co. Most of the code that currently is in RestIndicesGetAction should move to a TransportAction and the api should be added to the Client. Left other few comments, looks good in general though!

@rashidkpc
Copy link

I wonder if we should rationalize the responses between /_all/_whatever and /_whatever? I'm not totally clear how they're related, but we have a corresponding root endpoint for all of these. Some are pluralized, some are not, some return the same data in the same format (_mapping, _settings), others return in a different format. (_aliases, _warmer)

I noticed this doesn't remove /_aliases, however /index/_aliases returns a different format when no aliases are present than /_aliases. I wonder if we should return a key with an empty object for things that the user requests but don't exist, like _/aliases does:

/_aliases

   "logstash-2014.08.22": {
      "aliases": {}
   }

/_all/_aliases

   "logstash-2014.08.22": {}

There's also this difference between /_warmer and /_all/_warmer

/_warmer

{}

/_all/_warmer/

"logstash-2014.08.22": {}

Thoughts?

@@ -521,6 +525,26 @@
void aliasesExist(GetAliasesRequest request, ActionListener<AliasesExistResponse> listener);

/**
* Get specific index aliases that exists in particular indices and / or by name.
Copy link
Contributor

Choose a reason for hiding this comment

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

copy paste error?

$ curl -XGET 'http://localhost:9200/twitter/_settings,_mappings'
--------------------------------------------------

The above commend will only return the settings and mappings for the index called `twitter`.
Copy link
Member

Choose a reason for hiding this comment

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

s/commend/command

@javanna javanna removed the review label Sep 10, 2014
@@ -142,7 +142,7 @@ setup:
- match: {test_index2.mappings.test_type.properties.text.type: string}
- match: {test_index2.mappings.test_type.properties.text.analyzer: whitespace}

- is_false: foo
- match: { foo.mappings: {} }
Copy link
Member

Choose a reason for hiding this comment

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

Are these breaking changes properly documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added notes to the relevant pages and to the breaking changes in 1.4 page. This required me to rebase on master as the breaking changes page is new

@javanna
Copy link
Member

javanna commented Sep 10, 2014

This is very close, left a few minor comments. One more thing, we have quite some coverage through REST tests but now that the api is a proper client action I would love to see also some java integration tests for it.

private ImmutableOpenMap<String, ImmutableList<IndexWarmersMetaData.Entry>> warmers = ImmutableOpenMap.of();
private ImmutableOpenMap<String, ImmutableOpenMap<String, MappingMetaData>> mappings = ImmutableOpenMap.of();
private ImmutableOpenMap<String, ImmutableList<AliasMetaData>> aliases = ImmutableOpenMap.of();
private ImmutableOpenMap<String, Settings> settings = null;
Copy link
Member

Choose a reason for hiding this comment

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

why are settings null by default? What's the difference between settings and the other features?

@javanna
Copy link
Member

javanna commented Sep 11, 2014

LGTM thanks @colings86 !

Returns information about settings, aliases, warmers, and mappings. Basically returns the IndexMetadata. This new endpoint replaces the /{index}/_alias|_aliases|_mapping|_mappings|_settings|_warmer|_warmers and /_alias|_aliases|_mapping|_mappings|_settings|_warmer|_warmers endpoints whilst maintaining the same response formats.  The only exception to this is on the /_alias|_aliases|_warmer|_warmers endpoint which will now return a section for 'aliases' or 'warmers' even if no aliases or warmers exist. This backwards compatibility change is documented in the reference docs.

Closes #4069
@colings86 colings86 merged commit 5fe782b into elastic:master Sep 11, 2014
karmi added a commit to elastic/elasticsearch-ruby that referenced this pull request Sep 15, 2014
Related: elastic/elasticsearch#7234, elasticsearch/elasticsearch/#4069
@colings86 colings86 deleted the feature/getIndexAPI branch September 15, 2014 10:52
@rashidkpc
Copy link

Potentially major issue, the new API doesn't appear to support ignore_missing

Elasticsearch 1.3.2

curl -XGET  http://localhost:9200/logstash-2014.09.16,logstash-2014.09.17/_aliases?ignore_missing=true;echo
{"logstash-2014.09.16":{"aliases":{}}}

Elasticsearch 1.4 branch

$ curl -XGET  http://localhost:9200/logstash-2014.09.16,logstash-2014.09.17/_aliases?ignore_missing=true;echo
{"error":"IndexMissingException[[logstash-2014.09.17] missing]","status":404}

@javanna javanna removed the review label Sep 18, 2014
@colings86
Copy link
Contributor Author

@rashidkpc the ignore_missing was replaced in 1.x by the parameters described in [1]. The aliases api performs a cluster state request which doesn't allow setting indices options and is lenient (i.e. doesn't error on missing indices) so the ignore_missing option actually had no effect in 1.3.2. The difference now is that the GET Index API does support indices options and defaults to strictExpandOpen which is in line with the other APIs and so will error on missing indices. I will add the change to the breaking changes documentation but if you add the ignore_unavailable query parameter instead you should get the desired functionality.

[1] http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/_parameters.html#_parameters

@clintongormley clintongormley added the :Data Management/Indices APIs APIs to create and manage indices and templates label Jun 6, 2015
@clintongormley clintongormley changed the title Indices API: Added GET Index API Added GET Index API Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GET /index API
7 participants