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

Conversation

Projects
None yet
7 participants
@colings86
Copy link
Member

commented Aug 12, 2014

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

Closes #4069

@colings86

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2014

@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

This comment has been minimized.

Copy link
Contributor

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?

@javanna

View changes

docs/reference/indices/get-index.asciidoc Outdated

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

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

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

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 12, 2014

Member

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.

This comment has been minimized.

Copy link
@colings86

colings86 Aug 12, 2014

Author Member

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

- is_true: test_index.settings
- is_true: test_index_2.settings
- is_false: test_index.aliases
- is_false: test_index.warmers

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

Maybe add some negative tests, like what happens if the index is not there etc.? Also does the api support the ignore_unavailable flag & co.?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 13, 2014

Author Member

Added tests for this

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Great, thanks!

usedWildcard = true;
} else {
usedWildcard = token.equals(wildcard);
}

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

Can you elaborate on why this change was needed? Was it a bug that you found and fixed while working on this new api?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 13, 2014

Author Member

@javanna
The fix is to solve an issue introduced with this new API. This API binds to {index}/{feature} where feature is _settings, _mappings, _warmers, or _aliases, but we also have other APIs which bind to e.g. {index}/_settings/{type} etc. the settings API doesn't have a handler for {index}/_settings but before this change would have been chosen and ES would return an error saying there is no handler for {index}/_settings when actually we want it to pick the handler for {index}/{feature} in this case.

This fix now means that if we are requesting a/b and there is a handler for a/* and a handler for a/b/c , but no handler for a/b, the a/* handler will be chosen rather than returning an error.

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Ok sounds good....as long as all the REST tests pass, I'm always afraid of any change to PathTrie :)
Thanks a lot for the detailed explanation! Maybe it would be better to treat it as a bugfix and get it in separately? I don't have a strong opinion though since it's something that came up with this new api and wouldn't make sense without. Just thinking it would better highlighted as a different change and leave some more history on it this was applied.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestIndicesGetAction.java Outdated
builder.startObject();

ImmutableOpenMap<String, IndexMetaData> indexMetaDataMap = response.getState().metaData().indices();
String[] concreteIndices = response.getState().metaData().concreteIndices(IndicesOptions.fromOptions(true, true, true, false), indexes);

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

I think the api should support indices options as all other multi_index api, you just need to parse them from the request and pass them over as argument to the concreteIndices instead of hardcoding them.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestIndicesGetAction.java Outdated
return new BytesRestResponse(OK, builder);
}

private void writeWarmer(XContentBuilder builder, ImmutableList<IndexWarmersMetaData.Entry> entries, RestRequest request) throws IOException {

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

writeWarmers maybe instead of the singular?

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestIndicesGetAction.java Outdated
final ClusterStateRequest clusterStateRequest = new ClusterStateRequest();
clusterStateRequest.indices(indexes);
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
clusterStateRequest.listenerThreaded(false);

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2014

Member

I would make sure that we ask only the needed info here, not sure if we get back more than just indices metadata by default, I guess so

@javanna

This comment has been minimized.

Copy link
Member

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 colings86 added the review label Aug 13, 2014

@colings86

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2014

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

@javanna

View changes

rest-api-spec/api/indices.get.json Outdated
"type":"boolean",
"description":"Whether aliases pointing to multiple indices are allowed (default: false)"
},
"forbid_closed_indices":{

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

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

View changes

rest-api-spec/test/indices/get/10_basic.yaml Outdated
allow_no_indices: false

---
"Should index should return test_index_2 if expand_wildcards=open":

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Should index should return? :)

@javanna

View changes

rest-api-spec/test/indices/get/10_basic.yaml Outdated
- is_false: test_index_3.settings

---
"Should index should return test_index_3 if expand_wildcards=closed":

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Same as above

@javanna

View changes

rest-api-spec/test/indices/get/10_basic.yaml Outdated
- is_true: test_index_3.settings

---
"Should index should return test_index_2 and test_index_3 if expand_wildcards=open,closed":

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Same as above

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequest.java Outdated
@@ -126,6 +132,7 @@ public void readFrom(StreamInput in) throws IOException {
metaData = in.readBoolean();
blocks = in.readBoolean();
indices = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

I think I mislead you here. I meant to add support for indices options to the new api, not to cluster state api directly (which I'm not sure about and if done without version checks would also break bw comp). Have a look at the existing get settings api for instance or get aliases. The difference is that you are exposing a new REST api only (no transport action), thus you would need to call concrete indices yourself on the REST action. Now that I think about it though I'm wondering if it would make sense to expose this get index as a proper java api too, like get settings and co. What do you think?

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Also with this change you wouldn't really add support for indices options to the cluster state api, but only add the field to the request, which doesn't seem to belong here.

This comment has been minimized.

Copy link
@colings86

colings86 Aug 13, 2014

Author Member

I think you raise a good point. The problem with this change at the moment is that it only exposes the functionality on the REST layer and seems a bit messy with the reuse of the cluster state API. I think I will expose it as a proper java API as you suggest, putting it more in line with the other endpoints in the Indices API. That way the API can basically expose the IndexMetaData object which contains all the info including the warmers which have to be retrieved separately in this method.

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Sounds great, that's what I would do as well.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestIndicesGetAction.java Outdated
/**
*
*/
public class RestIndicesGetAction extends BaseRestHandler {

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

nitpicking, but maybe it should be RestGetIndicesAction which seems to be more compliant with the already existing REST actions?

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Member

@colings86 you didn't reply to this comment, is it because you disagree? :)

This comment has been minimized.

Copy link
@colings86

colings86 Sep 10, 2014

Author Member

Ah sorry, missed this one. Will update the name as you suggest.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestIndicesGetAction.java Outdated
builder.startObject();

ImmutableOpenMap<String, IndexMetaData> indexMetaDataMap = response.getState().metaData().indices();
String[] concreteIndices = response.getState().metaData().concreteIndices(indicesOptions, indexes);

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

not sure why you need to resolve indices only for warmers. Does it mean that if you call cluster state providing indices the returned warmers are not filtered? Or is it just the only way to get to the warmers?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 13, 2014

Author Member

Only way to get warmers as they are not in the cluster state response.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/warmer/get/RestGetWarmerAction.java Outdated
@@ -45,7 +45,6 @@ public RestGetWarmerAction(Settings settings, Client client, RestController cont
super(settings, client);
controller.registerHandler(GET, "/_warmer", this);
controller.registerHandler(GET, "/_warmer/{name}", this);
controller.registerHandler(GET, "/{index}/_warmer", this);

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

Aren't these removals of REST endpoints breaking? Can they stay and get removed on a separate PR, so that we isolate there the breaking changes and we decide which branches we want to apply them to?

This comment has been minimized.

Copy link
@colings86

colings86 Aug 13, 2014

Author Member

I don't think they are breaking as the endpoints still work and return the same content, they are just handled in the RestIndicesGetAction instead of in these files now.

This comment has been minimized.

Copy link
@javanna

javanna Aug 13, 2014

Member

sweet! and we should have REST tests for them which you haven't removed, so everything should be fine indeed

@javanna javanna removed the review label Aug 13, 2014

@javanna

This comment has been minimized.

Copy link
Member

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!

@colings86 colings86 self-assigned this Aug 13, 2014

@colings86 colings86 force-pushed the colings86:feature/getIndexAPI branch 2 times, most recently Aug 27, 2014

@colings86 colings86 added the review label Aug 27, 2014

@rashidkpc

This comment has been minimized.

Copy link
Member

commented Aug 27, 2014

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?

@spinscale

View changes

src/main/java/org/elasticsearch/client/IndicesAdminClient.java Outdated
@@ -521,6 +525,26 @@
void aliasesExist(GetAliasesRequest request, ActionListener<AliasesExistResponse> listener);

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

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 28, 2014

Member

copy paste error?

@spinscale

View changes

src/main/java/org/elasticsearch/client/IndicesAdminClient.java Outdated
ActionFuture<GetIndexResponse> getIndex(GetIndexRequest request);

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

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 28, 2014

Member

copy paste error?

@javanna

View changes

docs/reference/indices/get-index.asciidoc Outdated
$ curl -XGET 'http://localhost:9200/twitter/_settings,_mappings'
--------------------------------------------------

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

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Member

s/commend/command

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexRequest.java Outdated
private String[] features = new String[] { "_settings", "_warmers", "_mappings", "_aliases" };

public GetIndexRequest features(String[] features) {
this.features = features;

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Member

can we reject null here since we would break while serializing a null array?

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequest.java Outdated

public ClusterStateRequest indicesOptions(IndicesOptions indicesOptions) {
this.indicesOptions = indicesOptions;
return this;

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Member

I think these changes are not needed, maybe a leftover from the previous implementation?

@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: {} }

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Member

Are these breaking changes properly documented?

This comment has been minimized.

Copy link
@colings86

colings86 Sep 10, 2014

Author Member

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

This comment has been minimized.

Copy link
Member

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.

@colings86 colings86 force-pushed the colings86:feature/getIndexAPI branch Sep 10, 2014

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java Outdated
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;

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

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

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java Outdated
import org.elasticsearch.transport.TransportService;

/**
* Delete index action.

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

Leftover comment ;)

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java Outdated
case "_warmer":
case "_warmers":
if (!doneWarmers) {
warmersResult = state.metaData().findWarmers(concreteIndices, request.types(), new String[0]);

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

maybe use Strings.EMPTY_ARRAY instead of new String[0]?

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java Outdated
case "_alias":
case "_aliases":
if (!doneAliases) {
aliasesResult = state.metaData().findAliases(new String[0], concreteIndices);

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

Strings.EMPTY_ARRAY ?

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestGetIndicesAction.java Outdated
String[] features = request.paramAsStringArray("type", null);
// Work out if the indices is a list of features
if (features == null && indices.length > 0 && indices[0] != null && indices[0].startsWith("_") && !"_all".equals(indices[0]))
{

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

can you move the bracket to the previous line?

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestGetIndicesAction.java Outdated
writeWarmers(response.warmers().get(index), builder, request);
break;
default:
throw new IllegalStateException("feature [" + feature + "] is not valid");

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

Seems like this causes us to return an error if an unknown feature is requested, but only through the REST layer, while java API doesn't barf. Shall we make this consistent? Either ignore it in both places or throw error internally in the transport action?

@javanna

View changes

src/test/java/org/elasticsearch/action/admin/indices/get/GetIndexTests.java Outdated
}

@Test
public void testSimple_mapping() {

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Member

I think we prefer not to use underscores as part of method names, camel case is better

@javanna

This comment has been minimized.

Copy link
Member

commented Sep 11, 2014

LGTM thanks @colings86 !

@colings86 colings86 force-pushed the colings86:feature/getIndexAPI branch Sep 11, 2014

Indices API: Added GET Index API
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 force-pushed the colings86:feature/getIndexAPI branch to 5fe782b Sep 11, 2014

@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

[API] Added the "Get Indices Info" API
Related: elastic/elasticsearch#7234, elasticsearch/elasticsearch/#4069

@colings86 colings86 deleted the colings86:feature/getIndexAPI branch Sep 15, 2014

@rashidkpc

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

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

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2014

@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

@Mpdreamz Mpdreamz referenced this pull request Dec 8, 2014

Closed

Support get index API #1083

@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
You can’t perform that action at this time.