Move indices field from RankEvalSpec to RankEvalRequest by cbuescher · Pull Request #28341 · elastic/elasticsearch

cbuescher · 2018-01-23T16:25:08Z

Currently we store the indices specified in the request URL together with all
the other ranking evaluation specification in RankEvalSpec. This is not ideal
since e.g. the indices are not rendered to xContent and so cannot be parsed
back. Instead we should keep them in RankEvalRequest, like we already also do
for other kinds of requests (like SearchRequest).

javanna

I had not seen this, I left a couple of comments, LGTM though. Wasn't there also a TODO somewhere in the high-level client code that can be resolved once we merge this in?

javanna · 2018-02-28T16:08:40Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

in.readStringArray()?

I changed this.

javanna · 2018-02-28T16:09:01Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

out.writeStringArray()?

I changed this.

javanna · 2018-02-28T16:10:47Z

modules/rank-eval/src/test/resources/rest-api-spec/test/rank_eval/10_basic.yml

is this correct? why skip rest tests? aren't we only changing where indices are set internally?

I changed this previously because I changed the place where the indices strings for serialized/deserialized. However, I now think this isn't necessary since even though the location where the string array is writen changes, the overall ordering on the wire shouldn't change, thus there should be nothing breaking here. I removed the version skip changes in the last update.

cbuescher · 2018-02-28T16:44:30Z

@javanna thanks for looking into this, I will update the PR shortly

cbuescher · 2018-03-13T12:02:24Z

/cc @elastic/es-search-aggs

Currently we store the indices specified in the request URL together with all the other ranking evaluation specification in RankEvalSpec. This is not ideal since e.g. the indices are not rendered to xContent and so cannot be parsed back. Instead we should keep them in RankEvalRequest, like we already also do for other kinds of requests (like SearchRequest).

cbuescher · 2018-03-15T16:53:55Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalSpec.java

-        indices = new ArrayList<>(indicesSize);
-        for (int i = 0; i < indicesSize; i++) {
-            this.indices.add(in.readString());
-        }


This should me handled by out.writeStringArray(indices) in the request now, and shouldn't change the wire protocol so no version-dependent logic needed here I think.

I think we are out of luck, I hadn't noticed. readStringArray does readVInt while we were previously doing readInt, same for the write methods. This is odd, it sounds like it would break bw comp on the wire.

Good point, I will add a version check here.

cbuescher · 2018-03-15T16:54:15Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalSpec.java

-        out.writeInt(indices.size());
-        for (String index : indices) {
-            out.writeString(index);
-        }


This should me handled by in.readStringArray() in the request now, and shouldn't change the wire protocol so no version-dependent logic needed here I think.

cbuescher · 2018-03-15T16:55:39Z

@javanna thanks for the review, I pushed an update. Can you take a look again and also check if my above reasoning about not needing a bwc check on the wire makes sense?

javanna

left a small comment

javanna · 2018-03-15T18:24:32Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

+    }
+
+    public String[] getIndices() {
+        return Arrays.copyOf(indices, indices.length);


nit: just return the indices? I think that's what we do everywhere else.

I wanted to protect the indices from accidental modifications, but since there is a setter for them, they are mutable anyway, so I changed this like you suggested.

javanna · 2018-03-15T18:27:36Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalSpec.java

-        indices = new ArrayList<>(indicesSize);
-        for (int i = 0; i < indicesSize; i++) {
-            this.indices.add(in.readString());
-        }


I think we are out of luck, I hadn't noticed. readStringArray does readVInt while we were previously doing readInt, same for the write methods. This is odd, it sounds like it would break bw comp on the wire.

cbuescher · 2018-03-16T10:44:55Z

@javanna I added version checks and made a few more changes in my last updates, can you take another look?

javanna · 2018-03-16T16:10:59Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

+        if (in.getVersion().onOrAfter(Version.V_6_3_0)) {
+            indices = in.readStringArray();
+        } else {
+            // older 6.2 format


add to the comment readVint vs readInt just to clarify something that may not be clear at first?

I don't know if this clarifies things a lot since it requires looking into what readStringArray does? At this point I wonder if I should remove the comment completely, the code should explain this by itself and I think we can remove it on master after 6.3 is release (we only try to be bwc with the latest minor release before a major, no?)

I think the comment may be useful, to clarify why we have this conditional as it looks weird at first. Of course if one looks deeper will also find out why we have it. Up to you

javanna · 2018-03-16T16:11:06Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

+        if (out.getVersion().onOrAfter(Version.V_6_3_0)) {
+            out.writeStringArray(indices);
+        } else {
+            // older 6.2 format


javanna · 2018-03-16T16:12:15Z

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalRequest.java

    public void writeTo(StreamOutput out) throws IOException {
        super.writeTo(out);
        rankingEvaluationSpec.writeTo(out);
+        if (out.getVersion().onOrAfter(Version.V_6_3_0)) {


on the version conditionals, you may need to first use 7_0_0_alpha1 on master before you backport to 6.x, then you can update the version on master too.

Given that 6.3 isn't release yet, bwc tests should only run vs. 6.2 if this is merged shortly, or am I missing sth?

I always get this wrong, you seem to know what you are doing better than me! :)

np, I just thought about it when you mentioned it.

cbuescher · 2018-03-19T10:18:32Z

@javanna thanks for the review, I changed the comments slightly, will wait for this CI build before merging

cbuescher · 2018-03-19T11:55:51Z

@elasticmachine test this again please

Currently we store the indices specified in the request URL together with all the other ranking evaluation specification in RankEvalSpec. This is not ideal since e.g. the indices are not rendered to xContent and so cannot be parsed back. Instead we should keep them in RankEvalRequest.

cbuescher added >enhancement review v7.0.0 v6.3.0 :Search Relevance/Ranking Scoring, rescoring, rank evaluation. and removed :Java High Level REST Client labels Jan 23, 2018

cbuescher mentioned this pull request Jan 24, 2018

Add ranking evaluation API to High Level Rest Client #28357

Merged

javanna reviewed Feb 28, 2018

View reviewed changes

cbuescher self-assigned this Mar 13, 2018

cbuescher force-pushed the move-rankeval-indices branch from aa84707 to a34c6d3 Compare March 15, 2018 16:50

cbuescher commented Mar 15, 2018

View reviewed changes

javanna suggested changes Mar 15, 2018

View reviewed changes

Christoph Büscher added 2 commits March 16, 2018 11:17

Making indices a request argument

9ec476f

Adding version dependent serialization

45649d7

javanna reviewed Mar 16, 2018

View reviewed changes

javanna approved these changes Mar 16, 2018

View reviewed changes

Changing comment

868b2e3

Merge branch 'master' into move-rankeval-indices

ac5fb8c

cbuescher merged commit 8053222 into elastic:master Mar 19, 2018

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

cbuescher deleted the move-rankeval-indices branch November 27, 2025 10:47

Conversation

cbuescher commented Jan 23, 2018

Uh oh!

javanna left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Feb 28, 2018

Uh oh!

cbuescher commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Mar 15, 2018

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Mar 19, 2018

Uh oh!

cbuescher commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

javanna left a comment •

edited

Loading

cbuescher Mar 16, 2018 •

edited

Loading