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

Change behaviour of indices segments api to allow no indices #9219

Closed
wants to merge 4 commits into from

Conversation

@cbuescher
Copy link
Member

commented Jan 9, 2015

Writing the test for #5856 I found that when calling '_cat/segments' on an empty cluster returns {"error":"IndexMissingException[[_all] missing]","status":404} where other cat-commands like "shards" return silently. Although this is an edge case it would be more consistent with the other commands if "segments" would also do this.

In order to achieve this I had to change the IndicesOptions used in creating the IndicesSegmentsRequest to include the 'allowNoIndices' flag. Not sure if this breaks any other behaviour, though.

I added this edge case in the rest-test & let the regular mvn test run on this.

@@ -37,7 +37,7 @@ public IndicesSegmentsRequest() {

public IndicesSegmentsRequest(String... indices) {
super(indices);
indicesOptions(IndicesOptions.fromOptions(false, false, true, false));
indicesOptions(IndicesOptions.fromOptions(false, true, true, false));

This comment has been minimized.

Copy link
@javanna

javanna Jan 9, 2015

Member

this change affects also the default behaviour of indices segments api. I am not sure we should change that as well? Could you double check what is other apis default behaviour and see if only the cat variant sets different indices options to its own internal request, or if indices segments has a weird default indices options?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jan 12, 2015

Author Member

I checked two of the other cat apis for their internal treatment of the IndicesOptions, at least the "_cat/shards", "_cat/count" and "_cat/indices" return without exception when the cluster is empty. They internally use an IndicesStatsRequest which defaults to 'IndicesOptions.strictExpandOpenAndForbidClosed()' and that flavor of options setting has the 'allowNoIndices' set to true like in my proposed changes.

I also checked the current behaviour of the non-cat indices apis "_count", "_stats" which seem to be comparable to me. They internally also build requests (CountRequest, IndicesStatsRequest) which have the IndicesOptions.allowNoIndices flag turned on. "_segments" seems to differ for some reason, but I don't really see why. If there is a reason for the diverging behaviour of both "_segments" and "_cat/segments" there is no real need to change this. Anyone else more knowledgable in that regard?

This comment has been minimized.

Copy link
@javanna

javanna Jan 13, 2015

Member

Thanks a lot for checking! I did some digging too, to be honest I think this is a bug in IndicesSegmentsRequest. Shouldn't we just stick to the same default that comes from BroadcastOperationRequest? Would that differ from what you are doing?

Also, maybe we could update the description of the PR to make it clearer to users that we are changing the default behaviour of indices segments api too.

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 13, 2015

LGTM besides a little comment, @martijnvg do you mind double checking? The different indices segments default comes from #4453 I think, which you worked on.

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 13, 2015

@cbuescher can you also label this PR please?

@cbuescher cbuescher changed the title Allow '_cat/segments' to return with empty result when no index is present Change behaviour of indices segments api to allow no indices Jan 13, 2015
@cbuescher cbuescher self-assigned this Jan 13, 2015
@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2015

Just to re-check: the current setting in IndicesSegmentsRequest (IndicesOptions.fromOptions(false, false, true, false)) only sets the 'expandToOpenIndices' option. The current change in this PR would add the 'allowNoIndices' setting to that. The Default in other Requests extending BroadcastOperationRequest has the additional 'forbidClosedIndices' option set too. If that would be desirable for the indices segments api too, we could completely get rid of special IndicesOptions for IndicesSegmentsRequest and use that default.

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 13, 2015

I am +1 on using the default, that implies forbidClosedIndices set to true as well. One can always override the default if needed anyway. Thoughts @martijnvg ?

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 13, 2015

Thinking more about it.... forbidClosedIndices is not settable through REST but used mainly internally...thus not settable by users (apart from users using the java API). We need to dig what makes more sense for the indices segments api. Does it make sense to allow calling it for closed indices?

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2015

I did some more checking on the forbidClosedIndices option and added some tests. If we use the default settings then issuing an index segment request on a closed index raises an IndexClosedException. This behaviour can be changed by the user by setting the Rest-parameter for "ignoreUnavailable" to true. In that case the response is just empty.
I will push the version where I use the default settings. @javanna Can you have a look at the tests to see the behaviour in this case?

@cbuescher cbuescher added the review label Jan 21, 2015
@javanna

This comment has been minimized.

Copy link
Member

commented Jan 26, 2015

I had another look at this, especially comparing the output of _cat/shards with the output from _cat/segments. The main difference is the exception returned by cat segments (and indices segments) when executing against a closed index, as the default indices options are strict (ignore_unavailable set to false). Although this default probably makes sense for all other broadcast operation requests, it feels wrong with _cat/segments when comparing it with _cat/shards, as the latter is lenient instead, thus any closed index is simply ignored. Not sure what the indices segments api beahviour should be by default though.

@clintongormley would you mind helping us out here? :)

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

@javanna took a while to get my head around the various options. Am I right in saying that you would change _cat/segments to always ignore closed indices (in the same way that _cat/shards does, while leaving the _segments API to throw an exception when all of the resolved indices are closed?

If so, i think that makes sense.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

@javanna and I chatted about this - the actual question is what to do when the user specifies a concrete (ie no wildcards) closed index: throw an exception or ignore?

The _cat/shards API currently ignores, while the _segments and _cat/segments APIs throw an exception, which at least provides an explanation of why there is no output. I'm leaning towards keeping the exception.

Rather than just changing _cat/shards to be consistent, we should revisit all of these index options and defaults in a separate issue to make sure that they still make sense.

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

Thanks @clintongormley opened #9438


- do:
cluster.health:
wait_for_status: yellow

This comment has been minimized.

Copy link
@javanna

javanna Jan 27, 2015

Member

you can wait for green here as you have no replicas, no?


- do:
cluster.health:
wait_for_status: yellow

This comment has been minimized.

Copy link
@javanna

javanna Jan 27, 2015

Member

same as above


- do:
cluster.health:
wait_for_status: yellow

This comment has been minimized.

Copy link
@javanna

javanna Jan 27, 2015

Member

same as above

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

I did a final review, left a small comment around testing, besides that LGTM

@javanna javanna removed the review label Jan 29, 2015
@javanna

This comment has been minimized.

Copy link
Member

commented Feb 12, 2015

Any progress on this @cbuescher ? it should be ready as soon as the last few comments are addressed.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2015

@javanna Just changed tests according to your last comments. Anything else to do in terms of documentation here? Otherwise I will push and merge with 1.x branch.


- do:
cluster.health:
wait_for_status: green

This comment has been minimized.

Copy link
@javanna

javanna Feb 12, 2015

Member

can you double check if we need this wait for green? I'd say we don't

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 12, 2015

LGTM...left a tiny comment around waiting for green after closing an index in a test

@cbuescher cbuescher closed this in 41befaf Feb 12, 2015
cbuescher added a commit that referenced this pull request Feb 12, 2015
Using '_cat/segments' or the indices segments api without matching any index
now returns empty result instead of throwing IndexMissingException.

Closes #9219
@cbuescher cbuescher deleted the cbuescher:fix/5856-b branch Mar 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.