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

RankEvalRequest should implement IndicesRequest #29188

Merged
merged 3 commits into from Mar 22, 2018

Conversation

Projects
None yet
5 participants
@cbuescher
Copy link
Member

commented Mar 21, 2018

Change RankEvalRequest to implement IndicesRequest, so it gets treated
in a similar fashion to regular search requests e.g. by security.

RankEvalRequest should implement IndicesRequest
Change RankEvalRequest to implement IndicesRequest, so it gets treated
in a similar fashion to regular search requests e.g. by security.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2018

Pinging @elastic/es-search-aggs

return new NamedXContentRegistry(new RankEvalPlugin().getNamedXContent());
}

public void testSerialization() throws IOException {

This comment has been minimized.

Copy link
@javanna

javanna Mar 21, 2018

Member

we could implement equals and hashcode and extend from a different base class. Maybe not worth it?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Mar 21, 2018

Author Member

I did not implement this on purpose since it looked like we only would need this in tests and implementing equals/hashCode introduces some extra burden. Happy to add it if there are other usages of this except for testing.

This comment has been minimized.

Copy link
@colings86

colings86 Mar 21, 2018

Member

IMO the extra overhead of maintaining hashcode and equals on the request would be more than offset by having this test extend the AbstractSerializingTestCase base class since having it extend that class will ensure that the request object is tested in the same way as all other Request obbjects

This comment has been minimized.

Copy link
@javanna

javanna Mar 21, 2018

Member

++ I tend to agree. In all of our requests/responses equals and hashcode are really only used for testing, and there are already so many tests that could subclass some of our base classes rather than roll their own test methods.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Mar 21, 2018

Author Member

Looks like I'm overruled, although I tend to dislike the extended use of test class hierarchies to share bits of basic testing infra. I added a commit to adress this.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2018

@colings86 @javanna you seem to be in favour of hashcode/equals, so I added it. Can you take another look at the rest of the PR?

@cbuescher cbuescher merged commit e4b3007 into elastic:master Mar 22, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2018

Thanks @javanna

cbuescher added a commit that referenced this pull request Mar 22, 2018

RankEvalRequest should implement IndicesRequest (#29188)
Change RankEvalRequest to implement IndicesRequest, so it gets treated
in a similar fashion to regular search requests e.g. by security.

martijnvg added a commit that referenced this pull request Mar 26, 2018

Merge remote-tracking branch 'es/master' into ccr
* es/master: (27 commits)
  [Docs] Add rank_eval size parameter k (#29218)
  [DOCS] Remove ignore_z_value parameter link
  Docs: Update docs/index_.asciidoc (#29172)
  Docs: Link C++ client lib elasticlient (#28949)
  [DOCS] Unregister repository instead of deleting it (#29206)
  Docs: HighLevelRestClient#multiSearch (#29144)
  Add Z value support to geo_shape
  Remove type casts in logging in server component (#28807)
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878)
  REST : Split `RestUpgradeAction` into two actions (#29124)
  Add error file docs to important settings
  Add note to low-level client docs for DNS caching (#29213)
  Harden periodically check to avoid endless flush loop (#29125)
  Remove deprecated options for query_string (#29203)
  REST high-level client: add force merge API (#28896)
  Remove license information from README.textile (#29198)
  Decouple more classes from XContentBuilder and make builder strict (#29197)
  [Docs] Fix missing closing block in cluster/misc.asciidoc
  RankEvalRequest should implement IndicesRequest (#29188)
  Use EnumMap in ClusterBlocks (#29112)
  ...

martijnvg added a commit that referenced this pull request Mar 26, 2018

Merge remote-tracking branch 'es/6.x' into ccr-6.x
* es/6.x: (29 commits)
  [Docs] Add rank_eval size parameter k (#29218)
  Docs: Update docs/index_.asciidoc (#29172)
  Docs: Link C++ client lib elasticlient (#28949)
  Docs: HighLevelRestClient#multiSearch (#29144)
  [DOCS] Remove ignore_z_value parameter link
  Add Z value support to geo_shape
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878)
  REST : Split `RestUpgradeAction` into two actions (#29124)
  [DOCS] Unregister repository instead of deleting it (#29206)
  Remove type casts in logging in server component (#28807)
  Add error file docs to important settings
  Add note to low-level client docs for DNS caching (#29213)
  testShrinkAfterUpgrade should only set mapping.single_type if bwc version > 5.5.0
  Harden periodically check to avoid endless flush loop (#29125)
  REST high-level client: add force merge API (#28896)
  Remove license information from README.textile (#29198)
  Decouple more classes from XContentBuilder and make builder strict (#29197)
  Propagate mapping.single_type setting on shrinked index (#29202)
  [Docs] Fix missing closing block in cluster/misc.asciidoc
  RankEvalRequest should implement IndicesRequest (#29188)
  ...

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

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.