[ML][Data Frame] make response.count be total count of hits #43241
Conversation
|
Pinging @elastic/ml-core |
| @@ -153,16 +155,33 @@ public Response(List<DataFrameTransformStateAndStats> transformsStateAndStats, L | |||
| public Response(StreamInput in) throws IOException { | |||
| super(in); | |||
| transformsStateAndStats = in.readList(DataFrameTransformStateAndStats::new); | |||
| if (in.getVersion().onOrAfter(Version.CURRENT)) { | |||
There was a problem hiding this comment.
How does this work when you don't provide a concrete version (e.g. 7.3.0) but "CURRENT"?
There was a problem hiding this comment.
@przemekwitek CURRENT is whatever the latest version is in that branch. Honestly, I probably could make it 7.3.0 as there are no BWC tests for Dataframe transforms yet.
There was a problem hiding this comment.
@przemekwitek my personal workflow when doing things that could break BWC is:
- Set the serialization code to refer to
CURRENTfor the change - Merge that PR
- Prepare for backport with appropriate version branch and change
CURRENTto that version - Mute the appropriate BWC tests in Master
- Merge the backport
- Unmute and change from
CURRENTto real version in Master PR.
The reason I do it this way is so the code gets exercised through the BWC tests throughout the process. If I muted BWC in this PR, I could potentially miss other BWC test failures.
There was a problem hiding this comment.
Thanks for the explanation Ben!
| @@ -174,15 +193,15 @@ public void readFrom(StreamInput in) { | |||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | |||
| builder.startObject(); | |||
| toXContentCommon(builder, params); | |||
| builder.field(DataFrameField.COUNT.getPreferredName(), transformsStateAndStats.size()); | |||
| builder.field(DataFrameField.COUNT.getPreferredName(), totalCount == 0 ? transformsStateAndStats.size() : totalCount); | |||
There was a problem hiding this comment.
Can "0" be a valid value of totalCount? If so, totalCount should likely be nullable.
There was a problem hiding this comment.
It will always be the case that totalCount <= transformsStateAndStats.size().
I see what your concern, but given how this class is used, there are no use cases where totalCount is < transformsStateAndStats.size(). I will try to make this more obvious and safe in the class.
| expandedIds -> { | ||
| request.setExpandedIds(new HashSet<>(expandedIds)); | ||
| request.setNodes(DataFrameNodes.dataFrameTaskNodes(expandedIds, clusterService.state())); | ||
| expandedIdsAndHits -> { |
There was a problem hiding this comment.
s/expandedIdsAndHits/hitsAndExpandedIds/
to make variable name indicate the order of tuple elements?
|
run elasticsearch-ci/1 |
...in/java/org/elasticsearch/xpack/core/dataframe/action/GetDataFrameTransformsStatsAction.java
Outdated
Show resolved
Hide resolved
| @@ -235,7 +240,7 @@ public void expandTransformIds(String transformIdsExpression, PageParams pagePar | |||
| requiredMatches.unmatchedIdsString()))); | |||
| return; | |||
| } | |||
| foundIdsListener.onResponse(ids); | |||
| foundIdsListener.onResponse(new Tuple<>(totalHits, ids)); | |||
There was a problem hiding this comment.
[optional] Alternatively you could use Tuple.tuple factory method instead of a constructor here.
…43241) * [ML][Data Frame] make response.count be total count of hits * addressing line length check * changing response count for filters * adjusting serialization, variable name, and total count logic * making count mandatory for creation
…43389) * [ML][Data Frame] make response.count be total count of hits * addressing line length check * changing response count for filters * adjusting serialization, variable name, and total count logic * making count mandatory for creation
Even though we support pagination for certain resources, there is no information returned to the end user to tell them that they need to look in the next page of resources.
This PR adjusts the the
countreturned in theQueryPagetype objects, and in DataFrameStats to be the totalhits of the query ran. This will show the user how many total documents match their simple ID patterned query.