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

Expose the indices names in every action relates to if applicable #6933

Closed

Conversation

Projects
None yet
5 participants
@javanna
Copy link
Member

javanna commented Jul 20, 2014

If a request relates to indices, expose which ones it relates to in a generic manner.

@uboness

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java Outdated
/**
* Returns the indices the action relates to
*/
String[] relatedIndices();

This comment has been minimized.

Copy link
@uboness

uboness Jul 21, 2014

Contributor

maybe name it requestedIndices() to avoid confusion of what exactly is returned?

@uboness

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequestHelper.java Outdated

import org.elasticsearch.cluster.metadata.MetaData;

public final class IndicesRelatedRequestHelper {

This comment has been minimized.

Copy link
@uboness

uboness Jul 21, 2014

Contributor

maybe move this class into the IndicesRelatedRequest as a static class call Helper

alternatively, you can maybe add indicesOrAll helper method directly to MetaData?

@uboness

View changes

src/main/java/org/elasticsearch/action/admin/indices/warmer/put/PutWarmerRequest.java Outdated
@@ -98,6 +99,14 @@ public ActionRequestValidationException validate() {
}

@Override
public String[] relatedIndices() {
if (searchRequest == null) {
throw new IllegalArgumentException("unable to retrieve indices, search request is null");

This comment has been minimized.

Copy link
@uboness

uboness Jul 21, 2014

Contributor

it's not really IllegalArgumentException... maybe IllegalStateException? is there any logical execution path where searchRequest is null? if no, then maybe an assert will do?

This comment has been minimized.

Copy link
@javanna

javanna Jul 21, 2014

Author Member

good point, IllegalStateException makes more sense. I wouldn't use an assert as this can happen whenever one calls requestedIndices without having set a search request before. This is also related to whether we should rely on a validated request or not, since validate throws error when a search request is null. I think we should keep the exception and make it illegal state.

@uboness

View changes

src/main/java/org/elasticsearch/action/bulk/BulkRequest.java Outdated
assert request instanceof IndicesRelatedRequest;
String[] relatedIndices = ((IndicesRelatedRequest) request).relatedIndices();
if (relatedIndices.length != 1) {
throw new IllegalStateException("each request within a bulk should relate to one and only one index");

This comment has been minimized.

Copy link
@uboness

uboness Jul 21, 2014

Contributor

maybe an assert here instead? The API shouldn't give an option to put invalid requests into the bulk. If it does, we have to fix it somewhere else.

This comment has been minimized.

Copy link
@javanna

javanna Jul 21, 2014

Author Member

yep this makes indeed more sense as an assert

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jul 21, 2014

Added a few comments... overall, LGTM, but please have someone else look at it as well (it's 3:40am here... I might have missed something ;))

@javanna javanna added the review label Jul 21, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 21, 2014

Pushed some new commits to address @uboness review. Also strengthen the code a bit to make sure the newly added requestedIndices method doesn't break or throws a proper error when run on top of a non valid request.

@kimchy

View changes

src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeRequest.java Outdated
@Override
public String[] requestedIndices() {
if (index == null) {
return new String[0];

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 21, 2014

Member

we can return here Strings.EMPTY_ARRAY, same common to other places we have this.

@kimchy

View changes

src/main/java/org/elasticsearch/action/bulk/BulkRequest.java Outdated
assert relatedIndices.length == 1;
indices[i++] = relatedIndices[0];
}
return indices;

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 21, 2014

Member

how do we define reuqestedIndices? do we guarantee they are unique? If so, this should be a set and then return it. Same applies to other places where we gather indices, like aliases action.

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jul 21, 2014

Added some comments. I think that the contact of requestedIndices need to be better defined, if it allows for duplicate index names or not. If it does, it should be documented, and if not, then certain implementations should change to reflect it. I am leaning towards having it unique in the contact, but not heavily...., would love to hear why @uboness thinks

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 21, 2014

Pushed 2 more commits to address @kimchy 's review: clarified the docs and changed the api to not return duplicates, switched requestedIndices return type from String[] to Set<String>.

@kimchy

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java Outdated
/**
* Returns a set of unique indices the action relates to
*/
Set<String> requestedIndices();

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 21, 2014

Member

can this be an ImmutableSet?

@kimchy

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java Outdated
if (MetaData.isAllIndices(indices)) {
return Sets.newHashSet(MetaData.ALL);
}
return Sets.newHashSet(indices);

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 21, 2014

Member

if we do ImmutableSet, lets use ImmutableSet#copyOf, which is optimize for things like single element array (which I suspect will be very common)

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 21, 2014

Just pushed a new commit that addresses the last comment about using ImmutableSet

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 22, 2014

This should be close, @kimchy can you please have another look?

@s1monw

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java Outdated
*/
ImmutableSet<String> requestedIndices();

public static class Helper {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 23, 2014

Contributor

can this be a simple method

@s1monw

View changes

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java Outdated
* Needs to be implemented by all {@link org.elasticsearch.action.ActionRequest} subclasses that relate to
* one or more indices. Allows to retrieve which indices the action relates to.
*/
public interface IndicesRelatedRequest {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 23, 2014

Contributor

maybe just call it IndicesRequest

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 23, 2014

hey luca, I can see your intention on this PR but I think we should do this differently. I think the idea of IndicesRequest is good but it should generify the public String[] indices() method that we already have and clearly document it's semantics. This way it will only return what the user specified without any processing depending on the request. In a second step we should add public IndicesOptions indicesOptions() method to that interface and return them from each implementing class such that we can make decision how to resolve names etc on top of the simple API.

For requests like MultiSearchRequest we might wanna have a marker interface that allows us to retrieve the sub-requests and then figure out what we need to do in terms of name resolving. makes sense?

@s1monw s1monw removed the review label Jul 23, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 23, 2014

Makes sense @s1monw , pushed a new commit that tries to reuse the existing indices() method whenever possible.

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jul 23, 2014

Pushed two more commits to address composite requests and added indicesOptions() to the IndicesRequest interface. Ready for nother review round.

@javanna javanna added the review label Jul 23, 2014

@@ -135,6 +134,9 @@ public AliasActions filter(String filter) {
}

public void indices(String... indices) {
if (indices == null) {
throw new ElasticsearchIllegalArgumentException("indices must not be null");

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 24, 2014

Contributor

maybe we should put a public static final String ALL_INDICES = "_all"; on IndicesRequest and mention it in the error message?

This comment has been minimized.

Copy link
@javanna

javanna Jul 24, 2014

Author Member

This request doesn't support empty or null values (both are rejected during validation), and will never expand to _all. This change is just to avoid an NPE in the following loop if one does request.indices(null). I don't think _all is related.

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 24, 2014

Contributor

ok fine

@@ -87,6 +89,19 @@ public String index() {
return this.index;
}

@Override
public String[] indices() {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 24, 2014

Contributor

should we also prevent null here too?

This comment has been minimized.

Copy link
@javanna

javanna Jul 24, 2014

Author Member

I think it's ok here as the index in the analyze api is optional

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 24, 2014

Contributor

ok cool

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 24, 2014

I left some minor comments. other than that this LGTM

@s1monw s1monw removed the review label Jul 24, 2014

javanna added some commits Jul 21, 2014

switched requestedIndices return type from String[] to Set<String>, i…
…mproved the api to make sure and document that duplicates are not returned
Renamed IndicesRelatedRequest to IndicesRequest and reused indices me…
…thod already existing in most of the requests

Note that the following composite actions are not marked as IndicesRequest and will be addressed by exposing their subrequests in a generic manner on a separate change.
Addressed composite action by adding CompositeIndicesRequest interface
The requests that implement it are: MultiSearchRequest, MultiGetRequest, MultiTermVectorsRequest, BulkRequest, BenchmarkRequest, PercolateRequest, MultiPercolateRequest and MoreLikeThisRequest.
Added indices options to IndicesRequest interface, to express the dif…
…ferent behaviour depending on the api

For instance get api and index api hold a single index, which can be an alias, but both apis don't expand wilcards and an empty array is not allowed nor it becomes _all, which is what happens with most of the multiple indices apis, like search api.

Streamlined indices options to all the request where it makes sense, rather than leaving it implicit in the related TransportAction when indices get expanded (tipycally MetaData#concreteIndices or MetaData#concreteSingleIndex). Added IndicesOptions parameter to MetaData#concreteSingleIndex to make sure it is taken from the request, where the information belongs, instead of hardcoded within MetaData.

@javanna javanna closed this in d9ff42f Jul 24, 2014

javanna added a commit that referenced this pull request Jul 24, 2014

Internal: expose the indices names every action relates to if applicable
Added two new interfaces:
1) IndicesRequest that allows to retrieve the indices the request relates to in a generic manner, together with the indices options that tell how they are going to get resolved and expanded
2) CompositeIndicesRequest for compound requests that hold multiple indices request like MultiSearchRequest, MultiGetRequest, MultiTermVectorsRequest, BulkRequest, BenchmarkRequest, PercolateRequest, MultiPercolateRequest and MoreLikeThisRequest

Taken the chance to streamline the indices options and add them to every request where it makes sense (although they can't be changed from the outside), rather than leaving them implicit in the related TransportAction when indices get expanded (tipycally MetaData#concreteIndices or MetaData#concreteSingleIndex). Added IndicesOptions parameter to MetaData#concreteSingleIndex to make sure it is taken from the request, where the information belongs, instead of hardcoded within MetaData. The concreteSingleIndex method remains but it's just a utility method that returns a single index instead of an array and complains otherwise.

Also made sure NPE is never thrown when setting indices(null) to IndicesAliasesRequest, similar to what SearchRequest does.

Closes #6933

@javanna javanna self-assigned this Jul 24, 2014

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 24, 2014

Internal: split internal free context request used after scroll and s…
…earch

We currently use the same internal request when we need to free the search context after a search and a scroll. The two original requests though diverged after elastic#6933 as `SearchRequest` implements `IndicesRequest` while `SearchScrollRequest` and `ClearScrollRequest` don't. That said, with elastic#7319 we made `SearchFreeContextRequest` implement `IndicesRequest` by making it hold the original indices taken from the original request, which are null if the free context was originated by a scroll or by a clear scroll call, and that is why original indices are optional there.

This commit introduces a separate free context request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[free_context/scroll]` action that is equivalent to the previous `indices:data/read/search[free_context]` whose request implements now `IndicesRequest` and holds the original indices coming from the original request. The original indices in the latter requests can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 24, 2014

Internal: split internal free context request used after scroll and s…
…earch

We currently use the same internal request when we need to free the search context after a search and a scroll. The two original requests though diverged after elastic#6933 as `SearchRequest` implements `IndicesRequest` while `SearchScrollRequest` and `ClearScrollRequest` don't. That said, with elastic#7319 we made `SearchFreeContextRequest` implement `IndicesRequest` by making it hold the original indices taken from the original request, which are null if the free context was originated by a scroll or by a clear scroll call, and that is why original indices are optional there.

This commit introduces a separate free context request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[free_context/scroll]` action that is equivalent to the previous `indices:data/read/search[free_context]` whose request implements now `IndicesRequest` and holds the original indices coming from the original request. The original indices in the latter requests can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7856

javanna added a commit that referenced this pull request Sep 24, 2014

Internal: split internal free context request used after scroll and s…
…earch

We currently use the same internal request when we need to free the search context after a search and a scroll. The two original requests though diverged after #6933 as `SearchRequest` implements `IndicesRequest` while `SearchScrollRequest` and `ClearScrollRequest` don't. That said, with #7319 we made `SearchFreeContextRequest` implement `IndicesRequest` by making it hold the original indices taken from the original request, which are null if the free context was originated by a scroll or by a clear scroll call, and that is why original indices are optional there.

This commit introduces a separate free context request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[free_context/scroll]` action that is equivalent to the previous `indices:data/read/search[free_context]` whose request implements now `IndicesRequest` and holds the original indices coming from the original request. The original indices in the latter requests can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes #7856

javanna added a commit that referenced this pull request Sep 24, 2014

Internal: split internal free context request used after scroll and s…
…earch

We currently use the same internal request when we need to free the search context after a search and a scroll. The two original requests though diverged after #6933 as `SearchRequest` implements `IndicesRequest` while `SearchScrollRequest` and `ClearScrollRequest` don't. That said, with #7319 we made `SearchFreeContextRequest` implement `IndicesRequest` by making it hold the original indices taken from the original request, which are null if the free context was originated by a scroll or by a clear scroll call, and that is why original indices are optional there.

This commit introduces a separate free context request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[free_context/scroll]` action that is equivalent to the previous `indices:data/read/search[free_context]` whose request implements now `IndicesRequest` and holds the original indices coming from the original request. The original indices in the latter requests can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes #7856

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 25, 2014

Internal: split internal fetch request used within scroll and search
Similar to elastic#7856 but relates to the fetch shard level requests. We currently use the same internal request when we need to fetch within search and scroll. The two original requests though diverged after elastic#6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with elastic#7319 we made `FetchSearchRequest` implement IndicesRequest by making it hold the original indices taken from the original request, which are null if the fetch was originated by a search scroll, and that is why original indices are optional there.

This commit introduces a separate fetch request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[phase/fetch/id/scroll]` action that is equivalent to the previous `indices:data/read/search[phase/fetch/id]` whose request implements now IndicesRequest and holds the original indices coming from the original request. The original indices in the latter request can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 26, 2014

Internal: split internal fetch request used within scroll and search
Similar to elastic#7856 but relates to the fetch shard level requests. We currently use the same internal request when we need to fetch within search and scroll. The two original requests though diverged after elastic#6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with elastic#7319 we made `FetchSearchRequest` implement IndicesRequest by making it hold the original indices taken from the original request, which are null if the fetch was originated by a search scroll, and that is why original indices are optional there.

This commit introduces a separate fetch request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[phase/fetch/id/scroll]` action that is equivalent to the previous `indices:data/read/search[phase/fetch/id]` whose request implements now IndicesRequest and holds the original indices coming from the original request. The original indices in the latter request can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7870

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 26, 2014

Internal: split internal fetch request used within scroll and search
Similar to elastic#7856 but relates to the fetch shard level requests. We currently use the same internal request when we need to fetch within search and scroll. The two original requests though diverged after elastic#6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with elastic#7319 we made `FetchSearchRequest` implement IndicesRequest by making it hold the original indices taken from the original request, which are null if the fetch was originated by a search scroll, and that is why original indices are optional there.

This commit introduces a separate fetch request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[phase/fetch/id/scroll]` action that is equivalent to the previous `indices:data/read/search[phase/fetch/id]` whose request implements now IndicesRequest and holds the original indices coming from the original request. The original indices in the latter request can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7870

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 26, 2014

Internal: split internal fetch request used within scroll and search
Similar to elastic#7856 but relates to the fetch shard level requests. We currently use the same internal request when we need to fetch within search and scroll. The two original requests though diverged after elastic#6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with elastic#7319 we made `FetchSearchRequest` implement IndicesRequest by making it hold the original indices taken from the original request, which are null if the fetch was originated by a search scroll, and that is why original indices are optional there.

This commit introduces a separate fetch request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[phase/fetch/id/scroll]` action that is equivalent to the previous `indices:data/read/search[phase/fetch/id]` whose request implements now IndicesRequest and holds the original indices coming from the original request. The original indices in the latter request can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7870

@clintongormley clintongormley changed the title Internal: expose the indices names every action relates to if applicable Expose the indices names in every action relates to if applicable Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Internal: split internal free context request used after scroll and s…
…earch

We currently use the same internal request when we need to free the search context after a search and a scroll. The two original requests though diverged after elastic#6933 as `SearchRequest` implements `IndicesRequest` while `SearchScrollRequest` and `ClearScrollRequest` don't. That said, with elastic#7319 we made `SearchFreeContextRequest` implement `IndicesRequest` by making it hold the original indices taken from the original request, which are null if the free context was originated by a scroll or by a clear scroll call, and that is why original indices are optional there.

This commit introduces a separate free context request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[free_context/scroll]` action that is equivalent to the previous `indices:data/read/search[free_context]` whose request implements now `IndicesRequest` and holds the original indices coming from the original request. The original indices in the latter requests can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7856

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Internal: split internal fetch request used within scroll and search
Similar to elastic#7856 but relates to the fetch shard level requests. We currently use the same internal request when we need to fetch within search and scroll. The two original requests though diverged after elastic#6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with elastic#7319 we made `FetchSearchRequest` implement IndicesRequest by making it hold the original indices taken from the original request, which are null if the fetch was originated by a search scroll, and that is why original indices are optional there.

This commit introduces a separate fetch request and transport action for scroll, which doesn't hold original indices. The new action is only used against nodes that expose it, the previous action name will be used for nodes older than 1.4.0.Beta1.

As a result, in 1.4 we have a new `indices:data/read/search[phase/fetch/id/scroll]` action that is equivalent to the previous `indices:data/read/search[phase/fetch/id]` whose request implements now IndicesRequest and holds the original indices coming from the original request. The original indices in the latter request can only be null during a rolling upgrade (already existing version checks make sure that serialization is bw compatible), when some nodes are still < 1.4.

Closes elastic#7870
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.