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

[javanna]

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.

[s1monw]

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

[javanna]

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]

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?

[javanna]

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.

[s1monw]

then this entire thing makes not much sense to me really

[javanna]

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?

[s1monw]

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

[javanna]

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]

LGTM thanks luca