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

Make sure that all shard level requests hold the original indices #7319

Closed

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

javanna commented Aug 18, 2014

A request that relates to indices (IndicesRequest or CompositeIndicesRequest) might be converted to some other internal request(s) (e.g. shard level request) that get distributed over the cluster. Those requests contain the concrete index they refer to, but it is not known which indices (or aliases or expressions) the original request related to.

This commit makes sure that the original indices are available as part of the shard level requests and makes them implement IndicesRequest as well.

Also every internal request should be created passing in the original request, so that the original headers, together with the eventual original indices and options, get copied to it. Corrected some places where this information was lost.

NOTE: As for the bulk api and other multi items api (e.g. multi_get), their shard level requests won't keep around the whole set of original indices, but only the ones that related to the bulk items sent to each shard, the important bit is that we keep the original names though, not only the concrete ones.

Internal: make sure that all shard level requests hold the original i…
…ndices

A request that relates to indices (`IndicesRequest` or `CompositeIndicesRequest`) might be converted to some other internal request(s) (e.g. shard level request) that get distributed over the cluster. Those requests contain the concrete index they refer to, but it is not known which indices (or aliases or expressions) the original request related to.

This commit makes sure that the original indices are available as part of the shard level requests and makes them implement `IndicesRequest` as well.

Also every internal request should be created passing in the original request, so that the original headers, together with the eventual original indices and options, get copied to it. Corrected some places where this information was lost.

NOTE: As for the bulk api and other multi items api (e.g. multi_get), their shard level requests won't keep around the whole set of original indices, but only the ones that related to the bulk items sent to each shard, the important bit is that we keep the original names though, not only the concrete ones.

@javanna javanna self-assigned this Aug 18, 2014

if (in.getVersion().onOrAfter(Version.V_1_4_0)) {
return new OriginalIndices(in.readStringArray(), IndicesOptions.readIndicesOptions(in));
}
return new OriginalIndices();

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 20, 2014

Contributor

can this be a static instance

@@ -134,7 +134,7 @@ protected ShardSegments shardOperation(IndexShardSegmentRequest request) throws
return new ShardSegments(indexShard.routingEntry(), indexShard.engine().segments());
}

public static class IndexShardSegmentRequest extends BroadcastShardOperationRequest {
static class IndexShardSegmentRequest extends BroadcastShardOperationRequest {

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 20, 2014

Member

Out of curiosity, why make these non-public now?

This comment has been minimized.

Copy link
@javanna

javanna Aug 20, 2014

Author Member

not directly related to this change, it's just one of the many classes I went over and realized it didn't need to be public, could move to another PR

This comment has been minimized.

Copy link
@javanna

javanna Aug 20, 2014

Author Member

I reverted those changes and pushed them separately ;)

@@ -43,6 +44,7 @@
}

public BulkItemRequest(int id, ActionRequest request) {
assert request instanceof IndicesRequest;

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 20, 2014

Contributor

we should sync here with #6907 I think we should push this refactoring soon after this is in.

This comment has been minimized.

Copy link
@javanna

javanna Aug 20, 2014

Author Member

I'm not sure if the linked issue number is right, I don't get what the relation is between the two.

@@ -23,7 +23,6 @@
import com.carrotsearch.hppc.LongArrayList;
import org.elasticsearch.Version;
import org.elasticsearch.action.support.single.shard.SingleShardOperationRequest;
import org.elasticsearch.common.Nullable;

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 20, 2014

Contributor

can we please get a BWC test for multiget :)

@@ -114,6 +119,7 @@ public ShardSearchRequest(String index, int shardId, int numberOfShards, SearchT
this.shardId = shardId;
this.numberOfShards = numberOfShards;
this.searchType = searchType;
this.originalIndices = new OriginalIndices();

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 20, 2014

Contributor

since OrigianIndices is immutable I think we can simply use a static EMTPY instance here and in other places making the default ctor private?

This comment has been minimized.

Copy link
@javanna

javanna Aug 20, 2014

Author Member

good idea, will give this a try

@@ -736,6 +736,39 @@ public void testIndicesStats() {
assertThat(indicesStatsResponse.getIndices().containsKey("test"), equalTo(true));
}

@Test
public void testMultiGet() throws ExecutionException, InterruptedException {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 20, 2014

Contributor

w00t

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 20, 2014

I added some minor comments. I think it's very close!

javanna added some commits Aug 20, 2014

Made OriginalIndices empty constructor private and added EMPTY consta…
…nt instead

This allowed also to remove the missing member in OriginalIndices, since empty original indices are always represented by the new constant.
@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Aug 20, 2014

Updated according to feedback, ready for another review round

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 20, 2014

this LGTM thanks luca

@s1monw s1monw removed the review label Aug 20, 2014

javanna added a commit that referenced this pull request Aug 20, 2014

Internal: make sure that all shard level requests hold the original i…
…ndices

A request that relates to indices (`IndicesRequest` or `CompositeIndicesRequest`) might be converted to some other internal request(s) (e.g. shard level request) that get distributed over the cluster. Those requests contain the concrete index they refer to, but it is not known which indices (or aliases or expressions) the original request related to.

This commit makes sure that the original indices are available as part of the shard level requests and makes them implement `IndicesRequest` as well.

Also every internal request should be created passing in the original request, so that the original headers, together with the eventual original indices and options, get copied to it. Corrected some places where this information was lost.

NOTE: As for the bulk api and other multi items api (e.g. multi_get), their shard level requests won't keep around the whole set of original indices, but only the ones that related to the bulk items sent to each shard, the important bit is that we keep the original names though, not only the concrete ones.

Closes #7319

@javanna javanna closed this in 441c1c8 Aug 20, 2014

javanna added a commit that referenced this pull request Aug 20, 2014

Internal: adjusted internal requests visibility from public to packag…
…e private (redo)

was just reverted by mistake in the failed attempt of isolating the change and taking it out of #7319

javanna added a commit that referenced this pull request Aug 20, 2014

Internal: adjusted internal requests visibility from public to packag…
…e private (redo)

was just reverted by mistake in the failed attempt of isolating the change and taking it out of #7319

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

Internal: make sure that all shard level requests hold the original i…
…ndices

A request that relates to indices (`IndicesRequest` or `CompositeIndicesRequest`) might be converted to some other internal request(s) (e.g. shard level request) that get distributed over the cluster. Those requests contain the concrete index they refer to, but it is not known which indices (or aliases or expressions) the original request related to.

This commit makes sure that the original indices are available as part of the shard level requests and makes them implement `IndicesRequest` as well.

Also every internal request should be created passing in the original request, so that the original headers, together with the eventual original indices and options, get copied to it. Corrected some places where this information was lost.

NOTE: As for the bulk api and other multi items api (e.g. multi_get), their shard level requests won't keep around the whole set of original indices, but only the ones that related to the bulk items sent to each shard, the important bit is that we keep the original names though, not only the concrete ones.

Closes #7319

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

Internal: adjusted internal requests visibility from public to packag…
…e private (redo)

was just reverted by mistake in the failed attempt of isolating the change and taking it out of #7319

@clintongormley clintongormley changed the title Internal: make sure that all shard level requests hold the original indi... Internal: Make sure that all shard level requests hold the original indices Sep 8, 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: Make sure that all shard level requests hold the original indices Make sure that all shard level requests hold the original indices 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.