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

Split internal fetch request used within scroll and search #7870

Closed

Conversation

Projects
None yet
3 participants
@javanna
Copy link
Member

commented Sep 25, 2014

Similar to #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 #6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with #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.

Internal: split internal fetch request used within scroll and search
Similar to #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 #6933 as SearchRequest implements IndicesRequest while SearchScrollRequest doesn't. That said, with #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.
/**
*
*/
public class FetchRequest extends TransportRequest {

This comment has been minimized.

Copy link
@uboness

uboness Sep 25, 2014

Contributor

can we rename it to ShardFetchRequest?

review round: renamed `FetchRequest` to `ShardFetchRequest` and `Fetc…
…hSearchRequest` to `ShardFetchSearchRequest`
@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2014

Addressed feedback, ready for another review round

@@ -39,7 +35,7 @@
/**
*

This comment has been minimized.

Copy link
@uboness

uboness Sep 26, 2014

Contributor

can we add docs here?

import java.io.IOException;

/**
*

This comment has been minimized.

Copy link
@uboness

uboness Sep 26, 2014

Contributor

Can we add docs here and explain why we need it in addition to ShardFetchRequest?

@uboness

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

Added a couple of comments for documentation, other than that LGTM

@uboness uboness removed the review label Sep 26, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2014

Added docs @uboness , you mind having another look please?

@uboness

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

LGTM

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 javanna closed this in e85e079 Sep 26, 2014

@clintongormley clintongormley changed the title Internal: split internal fetch request used within scroll and search Split internal fetch request used within scroll and search Jun 7, 2015

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.