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

Clarify when a shard search request gets created to be only used locally #7855

Closed

Conversation

Projects
None yet
3 participants
@javanna
Copy link
Member

commented Sep 24, 2014

In some cases a shard search request gets created on a node to be only used there and never sent over the transport. This commit clarifies that and creates a new base class called ShardSearchLocalRequest that can and will be only used locally. ShardSearchTransportRequest on the other hand delegates to the local version but extends TransportRequest and is Streamable, which means that it is supposed to be sent over the transport.

This way we can make the OriginalIndices only required (and mandatory now) in the transport variant.

Took the chance to remove an unused InternalScrollSearchRequest constructor and an empty else branch in TransportSearchScrollQueryAndFetchAction.

@javanna javanna self-assigned this Sep 24, 2014

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch Sep 24, 2014

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch Sep 24, 2014

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch Sep 24, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2014

wouldn't it make more sense to have a base class SearchShardRequest and a TransportSearchShardRequest that also implements streamable instead of using factories?

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch 2 times, most recently Sep 26, 2014

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch Sep 26, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2014

Hey @s1monw your comment makes sense to me. It just was not as simple as that given that the transport version needs to extend TransportRequest, not just implement Streamable. I did split the requests but had to make the existing class an interface that is implemented by both. Much much cleaner but slightly bigger change than what I had in mind at first ;) Let me know what you think!

@s1monw

View changes

src/main/java/org/elasticsearch/search/internal/ShardSearchLocalRequest.java Outdated
}

@SuppressWarnings("unchecked")
void readFrom(StreamInput in) throws IOException {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 26, 2014

Contributor

to make this clean can we make all the members protected and then inside the ShardSearchTransportRequest we have a inner class that extends this local request and defines the readFrom / writeTo methods?

This comment has been minimized.

Copy link
@javanna

javanna Sep 27, 2014

Author Member

The thing is that this is indeed a local request and doesn't need to be Streamable, but the writeTo is somehow needed to locally build the cache key, that is why I kept the readFrom too in the same class here as they are related, and made them both package private. I agree it could be cleaner, not sure how...since both local and transport request need to write the cache key, which must be the same.

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 29, 2014

Contributor

then this entire thing makes not much sense to me really

This comment has been minimized.

Copy link
@javanna

javanna Sep 29, 2014

Author Member

I think it clarifies things, makes it clear when the shard search request is used locally and when it's sent over the transport. Better than static factories cause you can't now send ShardSearchLocalRequest over the transport at all as it doesn't implement TransportRequest. Also makes it more explicit that the binary representation is used as a cache key.

Maybe if things don't make sense it's because the way we use ShardSearchRequest is confusing per se and this PR doesn't clean it all up. How do you suggest we can improve this further?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 7, 2014

Contributor

well if you use the internal class you can have a static writeTo method that can be used to build a cache key?

@javanna javanna added v1.4.0 and removed v1.4.0.Beta1 labels Sep 29, 2014

@s1monw s1monw removed the review label Oct 7, 2014

@javanna javanna added the review label Oct 8, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

I pushed a new commit that should address your comment @s1monw . Can you also double check backwards compatibility please now that 1.4.0.Beta1 is released? We added originalIndices in beta1, but I think it makes no difference since the local variant of the request (where we now removed originalIndices) was never sent over the transport. The cache key does change (since it doesn't contain original indices anymore) but I think that is fine. And now originalIndices appear only where needed and are mandatory there (while before they were optional).

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

LGTM thanks luca

Internal: split shard search request into a local and a transport var…
…iant

In some cases a shard search request gets created on a node to be only used there and never sent over the transport. This commit clarifies that and creates a new base class called `ShardSearchLocalRequest` that can and will be only used locally. `ShardSearchTransportRequest` on the other hand delegates to the local version but extends `TransportRequest` and is `Streamable`, which means that it is supposed to be sent over the transport.

This way we can make the `OriginalIndices` only required (and mandatory now) in the transport variant.

Took the chance to remove an unused InternalScrollSearchRequest constructor and an empty else branch in `TransportSearchScrollQueryAndFetchAction`.

Closes #7855

@javanna javanna force-pushed the javanna:enhancement/shard_search_request_cleanup branch to 021cb84 Oct 8, 2014

javanna added a commit to javanna/elasticsearch that referenced this pull request Oct 8, 2014

Internal: split shard search request into a local and a transport var…
…iant

In some cases a shard search request gets created on a node to be only used there and never sent over the transport. This commit clarifies that and creates a new base class called `ShardSearchLocalRequest` that can and will be only used locally. `ShardSearchTransportRequest` on the other hand delegates to the local version but extends `TransportRequest` and is `Streamable`, which means that it is supposed to be sent over the transport.

This way we can make the `OriginalIndices` only required (and mandatory now) in the transport variant.

Took the chance to remove an unused InternalScrollSearchRequest constructor and an empty else branch in `TransportSearchScrollQueryAndFetchAction`.

Closes elastic#7855

javanna added a commit to javanna/elasticsearch that referenced this pull request Oct 8, 2014

Internal: split shard search request into a local and a transport var…
…iant

In some cases a shard search request gets created on a node to be only used there and never sent over the transport. This commit clarifies that and creates a new base class called `ShardSearchLocalRequest` that can and will be only used locally. `ShardSearchTransportRequest` on the other hand delegates to the local version but extends `TransportRequest` and is `Streamable`, which means that it is supposed to be sent over the transport.

This way we can make the `OriginalIndices` only required (and mandatory now) in the transport variant.

Took the chance to remove an unused InternalScrollSearchRequest constructor and an empty else branch in `TransportSearchScrollQueryAndFetchAction`.

Closes elastic#7855

@javanna javanna removed the review label Oct 8, 2014

@javanna javanna closed this in 3274e7e Oct 8, 2014

@clintongormley clintongormley changed the title Internal: clarify when a shard search request gets created to be only used locally Internal: Clarify when a shard search request gets created to be only used locally Nov 3, 2014

@clintongormley clintongormley changed the title Internal: Clarify when a shard search request gets created to be only used locally Clarify when a shard search request gets created to be only used locally Jun 7, 2015

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

Internal: split shard search request into a local and a transport var…
…iant

In some cases a shard search request gets created on a node to be only used there and never sent over the transport. This commit clarifies that and creates a new base class called `ShardSearchLocalRequest` that can and will be only used locally. `ShardSearchTransportRequest` on the other hand delegates to the local version but extends `TransportRequest` and is `Streamable`, which means that it is supposed to be sent over the transport.

This way we can make the `OriginalIndices` only required (and mandatory now) in the transport variant.

Took the chance to remove an unused InternalScrollSearchRequest constructor and an empty else branch in `TransportSearchScrollQueryAndFetchAction`.

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