-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Clarify when a shard search request gets created to be only used locally #7855
Conversation
82b3186
to
3aa887e
Compare
3aa887e
to
20e409e
Compare
20e409e
to
965950f
Compare
wouldn't it make more sense to have a base class |
f220806
to
b79bdfe
Compare
b79bdfe
to
316010a
Compare
Hey @s1monw your comment makes sense to me. It just was not as simple as that given that the transport version needs to extend |
} | ||
|
||
@SuppressWarnings("unchecked") | ||
void readFrom(StreamInput in) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this entire thing makes not much sense to me really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well if you use the internal class you can have a static writeTo method that can be used to build a cache key?
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 |
LGTM thanks luca |
…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
71588cf
to
021cb84
Compare
…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
…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
…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
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 extendsTransportRequest
and isStreamable
, 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
.