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

javanna · 2014-07-20T01:10:03Z

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

uboness · 2014-07-21T01:22:13Z

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java

+    /**
+     * Returns the indices the action relates to
+     */
+    String[] relatedIndices();


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

uboness · 2014-07-21T01:39:42Z

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 · 2014-07-21T11:12:46Z

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 · 2014-07-21T11:53:03Z

src/main/java/org/elasticsearch/action/admin/indices/analyze/AnalyzeRequest.java

+    @Override
+    public String[] requestedIndices() {
+        if (index == null) {
+            return new String[0];


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

kimchy · 2014-07-21T11:59:58Z

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 · 2014-07-21T14:46:14Z

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 · 2014-07-21T14:50:24Z

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java

+    /**
+     * Returns a set of unique indices the action relates to
+     */
+    Set<String> requestedIndices();


can this be an ImmutableSet?

javanna · 2014-07-21T15:35:02Z

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

javanna · 2014-07-22T09:05:33Z

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

s1monw · 2014-07-23T09:13:38Z

src/main/java/org/elasticsearch/action/IndicesRelatedRequest.java

+     */
+    ImmutableSet<String> requestedIndices();
+
+    public static class Helper {


can this be a simple method

s1monw · 2014-07-23T09:58:34Z

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?

javanna · 2014-07-23T12:49:35Z

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

javanna · 2014-07-23T17:38:50Z

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

s1monw · 2014-07-24T09:02:33Z

src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java

@@ -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");


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

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.

s1monw · 2014-07-24T09:05:56Z

I left some minor comments. other than that this LGTM

…mproved the api to make sure and document that duplicates are not returned

…ex and delete mapping

…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.

The requests that implement it are: MultiSearchRequest, MultiGetRequest, MultiTermVectorsRequest, BulkRequest, BenchmarkRequest, PercolateRequest, MultiPercolateRequest and MoreLikeThisRequest.

…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.

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

…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.

…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

…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

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.

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

…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

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 enhancement labels Jul 20, 2014

uboness reviewed Jul 21, 2014
View reviewed changes

javanna added the review label Jul 21, 2014

kimchy reviewed Jul 21, 2014
View reviewed changes

s1monw reviewed Jul 23, 2014
View reviewed changes

s1monw removed the review label Jul 23, 2014

javanna added the review label Jul 23, 2014

s1monw reviewed Jul 24, 2014
View reviewed changes

s1monw removed the review label Jul 24, 2014

Internal: expose the indices names every action relates to if applicable

eddb909

javanna added 9 commits July 24, 2014 12:05

switched requestedIndices return type from String[] to Set<String>, i…

870b8ea

…mproved the api to make sure and document that duplicates are not returned

switched to ImmutableSet for requestedIndices signature

e81cbdd

aligned requestedIndices to validate for open/close index, delete ind…

fbe463d

…ex and delete mapping

Addressed composite action by adding CompositeIndicesRequest interface

4298a8d

The requests that implement it are: MultiSearchRequest, MultiGetRequest, MultiTermVectorsRequest, BulkRequest, BenchmarkRequest, PercolateRequest, MultiPercolateRequest and MoreLikeThisRequest.

clarified what we do in MoreLikeThisRequest, also fixed behaviour

912dac3

improved PutWarmerRequest error message

efa44aa

used Item#indicesOptions in MultiGet

56d6fef

javanna closed this in d9ff42f Jul 24, 2014

javanna self-assigned this Jul 24, 2014

javanna mentioned this pull request Sep 24, 2014

Split internal free context request used after scroll and search #7856

Closed

javanna mentioned this pull request Sep 25, 2014

Split internal fetch request used within scroll and search #7870

Closed

clintongormley added the :Internal label Jun 7, 2015

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

javanna commented Jul 20, 2014

uboness Jul 21, 2014

uboness commented Jul 21, 2014

javanna commented Jul 21, 2014

kimchy Jul 21, 2014

kimchy commented Jul 21, 2014

javanna commented Jul 21, 2014

kimchy Jul 21, 2014

javanna commented Jul 21, 2014

javanna commented Jul 22, 2014

s1monw Jul 23, 2014

s1monw commented Jul 23, 2014

javanna commented Jul 23, 2014

javanna commented Jul 23, 2014

s1monw Jul 24, 2014

javanna Jul 24, 2014

s1monw Jul 24, 2014

s1monw commented Jul 24, 2014

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

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

Conversation

javanna commented Jul 20, 2014

uboness Jul 21, 2014

Choose a reason for hiding this comment

uboness commented Jul 21, 2014

javanna commented Jul 21, 2014

kimchy Jul 21, 2014

Choose a reason for hiding this comment

kimchy commented Jul 21, 2014

javanna commented Jul 21, 2014

kimchy Jul 21, 2014

Choose a reason for hiding this comment

javanna commented Jul 21, 2014

javanna commented Jul 22, 2014

s1monw Jul 23, 2014

Choose a reason for hiding this comment

s1monw commented Jul 23, 2014

javanna commented Jul 23, 2014

javanna commented Jul 23, 2014

s1monw Jul 24, 2014

Choose a reason for hiding this comment

javanna Jul 24, 2014

Choose a reason for hiding this comment

s1monw Jul 24, 2014

Choose a reason for hiding this comment

s1monw commented Jul 24, 2014