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

mpercolate api should serialise start time #15938

Merged
merged 1 commit into from Jan 15, 2016

Conversation

Projects
None yet
2 participants
@martijnvg
Copy link
Member

commented Jan 12, 2016

PR for #15908

(when back porting to other branches I'll add version checks for serialisation)

shardRequest.source(in.readBytesReference());
shardRequest.docSource(in.readBytesReference());
shardRequest.onlyCount(in.readBoolean());
PercolateShardRequest shardRequest = new PercolateShardRequest();

This comment has been minimized.

Copy link
@javanna

javanna Jan 13, 2016

Member

don't we need to read the original indices anymore here? not sure, but if that's the case we may be able to remove that specific PercolateShardRequest constructor.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jan 13, 2016

Author Member

good point. We need to take the original indices into account. Let me change this.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jan 13, 2016

Author Member

I think this change is ok and we do take the original indices into account. We do this when we create PercolateShardRequest instance in TransportMultiPercolateAction at line 206.

This comment has been minimized.

Copy link
@javanna

javanna Jan 13, 2016

Member

you are right, the original indices are read/written in the super class, good! You can then remove the PercolateShardRequest that takes shardId and originalIndices as arguments I think.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jan 13, 2016

Author Member

I tried to remove it but unit tests are using it (PercolateDocumentParserTests), the percolator document parser relies on the shard id to be set on the percolate shard request. So lets keep it?

This comment has been minimized.

Copy link
@javanna

javanna Jan 13, 2016

Member

I think it's odd to have a public constructor for a test only... remove the OriginalIndices parameter at least? it's always set to null I think.

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2016

@javanna I've updated this PR.

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

LGTM

@martijnvg martijnvg force-pushed the martijnvg:mpercolate/propagate_starttime branch Jan 14, 2016

@martijnvg martijnvg force-pushed the martijnvg:mpercolate/propagate_starttime branch to a05ea53 Jan 15, 2016

@martijnvg martijnvg merged commit a05ea53 into elastic:master Jan 15, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
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.