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

Indices API: fixes GET Alias API backwards compatibility #7892

Merged
merged 1 commit into from Sep 30, 2014

Conversation

Projects
None yet
3 participants
@colings86
Copy link
Member

colings86 commented Sep 26, 2014

For just the case when only the aliases are requested, the default indices options are to ignore missing indexes. When requesting any other feature or any combination of features, the default will be to error on missing indices.

Closes #7793

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestGetIndicesAction.java Outdated
defaultIndicesOptions = IndicesOptions.lenientExpandOpen();
}
}
getIndexRequest.indicesOptions(IndicesOptions.fromRequest(request, defaultIndicesOptions));

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

I wonder if this specific default should only be used in the REST layer, or if we should move this logic to the transport action so that it's applied to the java client as well. That way we would have consistency between REST and transport layer...

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

even better have this logic in the GetIndexRequest#indicesOptions getter maybe so that it returns the actual indices options that depends on the features?

This comment has been minimized.

Copy link
@colings86

colings86 Sep 26, 2014

Author Member

I don't think we should move it to the GetIndexRequest#indicesOptions getter as these are only the defaults and need to be overridden by the users request. I agree that we should have the same behaviour on both the REST and the transport layer so I'll move this logic to TransportGetIndexAction

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

I'd try to keep what gets returned from there from the request inline with what is actually used in the transport action. That's why I suggested to move the logic to the request.

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestGetIndicesAction.java Outdated
if (features != null && features.length == 1 && features[0] != null && ("_alias".equals(features[0]) || "_aliases".equals(features[0]))) {
// If we are asking for all indices we need to return open and closed, if not we only expand to open
if (indices.length == 1 && indices[0] != null && "_all".equals(indices[0])) {
defaultIndicesOptions = IndicesOptions.fromOptions(true, true, true, true);

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

I don't fully understand why we need this if, what makes this api different when it comes to resolving _all ?

This comment has been minimized.

Copy link
@colings86

colings86 Sep 26, 2014

Author Member

@clintongormley pointed out that in 1.3.1 if you call /_aliases it will return both open and closed indices, but if you call /foo*/_aliases it will only expand to open indices. This if statement is providing that behaviour

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

sneaky!

@javanna

View changes

src/main/java/org/elasticsearch/rest/action/admin/indices/get/RestGetIndicesAction.java Outdated
// This makes sure that the get aliases API behaves exactly like in previous versions wrt indices options iff only alises are requested
if (features != null && features.length == 1 && features[0] != null && ("_alias".equals(features[0]) || "_aliases".equals(features[0]))) {
// If we are asking for all indices we need to return open and closed, if not we only expand to open
if (indices.length == 1 && indices[0] != null && "_all".equals(indices[0])) {

This comment has been minimized.

Copy link
@javanna

javanna Sep 26, 2014

Member

you can probably use MetaData.isAllIndices?

@colings86 colings86 force-pushed the colings86:fix/getAliasesbwc branch 3 times, most recently Sep 29, 2014

@colings86 colings86 added the blocker label Sep 30, 2014

@javanna

View changes

src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexRequest.java Outdated
private IndicesOptions resolveIndicesOptions() {
IndicesOptions defaultIndicesOptions = IndicesOptions.strictExpandOpen();
String[] indices = indices();
// This makes sure that the get aliases API behaves exactly like in previous versions wrt indices options iff only alises are requested

This comment has been minimized.

Copy link
@javanna

javanna Sep 30, 2014

Member

s/alises/aliases

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Sep 30, 2014

I think we test this different behaviour for aliases already in the REST tests right? Shall we also add a small java test for it? Apart from that LGTM!

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Sep 30, 2014

LGTM

Indices API: fixes GET Alias API backwards compatibility
For just the case when only the aliases are requested, the default indices options are to ignore missing indexes. When requesting any other feature or any combination of features, the default will be to error on missing indices.

Closes #7793

@colings86 colings86 force-pushed the colings86:fix/getAliasesbwc branch to 7c7c80f Sep 30, 2014

@colings86 colings86 merged commit 7c7c80f into elastic:1.x Sep 30, 2014

@colings86 colings86 deleted the colings86:fix/getAliasesbwc branch Sep 30, 2014

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.