Move runtime fields stats to server (#69487) #69542
Conversation
Runtime fields usage is currently reported as part of the xpack feature usage API. Now that runtime fields are part of server, their corresponding stats can be moved to be part of the ordinary mapping stats exposed by the cluster stats API.
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
I had some small comments as I was getting up to speed with the approach.
...e/src/main/java/org/elasticsearch/xpack/core/runtimefields/RuntimeFieldsFeatureSetUsage.java
Show resolved
Hide resolved
| public RuntimeFieldsFeatureSetUsage(StreamInput in) throws IOException { | ||
| super(in); | ||
| this.stats = in.readList(RuntimeFieldStats::new); | ||
| super(in.getVersion().before(Version.V_7_13_0) ? in.readString() : "runtime_fields", |
There was a problem hiding this comment.
I'm wondering why we need to specialize the logic to the version. Could we adopt a simpler approach of always reading/ writing the parameters?
There was a problem hiding this comment.
good one, I guess I initially went for conditionally serializing just to make sure, but based on my understanding of how xpack usage works, this should not be needed. The important bit is that we never go and serialize this class to 8.0 which won't have the named writeable registered, but knowing that only 7.11 and 7.12 nodes may cause its serialization which are not 7.last, we should be ok because 7.11 and 7.12 will never be in the same cluster as 8.0 nodes. That said keeping the named writeable around for the whole 7.x series should do the trick. We could make it a no-op when this class gets serialized to/from 7.13+ but that should make no difference. Don't take my word on this though, were you thinking along the same lines? :)
There was a problem hiding this comment.
Yes this is exactly what I was thinking. For flattened fields we just kept the class around for the 7.x series, without any changes to its serialization logic.
javanna
added
the
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
This looks the same as #69487 but with conditionalized logic for streams. I don't quite understand what the plan there is. Normally the master PR would contain versioned logic as well, to match this. What is the plan for committing these without breaking CI?
|
hey @rjernst thanks for taking a look, I had left a note on what the plan is in the other PR, let me paste it here too and expand a bit. I was thinking that getting the 7.x change first should address the bwc tests currently failing on the master PR. I am handling backwards compatibility in this PR at the wire level so that things don't break in a mixed 7.11/12 and 7.13 cluster, but I am effectively removing the feature in 7.x, hence there should be no need for backwards compatibility in master, as 7.last won't contain the runtime fields plugin and the runtime fields telemetry will already be entirely part of cluster/stats. This is unusual like you noticed, normally the versioned logic would be in master, but in this case it is in 7.x. It is not even versioned logic as keeping the named writeable registered in 7.x should be enough to prevent things from breaking at the transport layer when 7.13+ deserializes from 7.11 or 7.12. |
Normally the versioned logic would be in both places initially, and would then be removed in master in a followup. The commit order is done this way to ensure bwc tests do not break even temporarily, so as not to create tons of CI failures that must be investigated. Committing 7.x first does not work because of how master selects the commit to build for bwc testing. It looks at the commit timestamps, and selects the commit just before the timestamp of the current commit on master. So committing to 7.x first would cause master to see the new commit in its next bwc test, but it would not yet have the logic for writing the new data. I think we should keep with normal bwc convention, committing to master, then 7.x, then fixing up master afterwards. |
|
that makes sense @rjernst I oversimplified backporting this one, thanks for refreshing my memory on this, I will take care of it. |
4 participants
This is the backport PR of #69487 , but it is not quite a simple backport, as it handle backwards compatibility to some degree, meaning making sure that communication betweenm 7.x nodes does not break when using the xpack usage API. On the other hand, we are effectively removing the runtime_fields feature section from the response of the xpack usage API in favour of getting the runtime fields stats from the cluster stats API.
Runtime fields usage is currently reported as part of the xpack feature usage API. Now that runtime fields are part of server, their corresponding stats can be moved to be part of the ordinary mapping stats exposed by the cluster stats API.
This change requires dealing with backwards compatibility on the transport layer. Xpack feature usage supports a minimum version, which is 7.11 for runtime fields, but we are going to remove the runtime fields portion from xpack feature usage starting from 7.13, as the corresponding output becomes part of the cluster stats API. That means that 8.0 (master) does not have to account for the potential presence of such section (as it will only talk to 7.last which is > 7.13), but 7.x does (when 7.13 talks to 7.12 and 7.11 it has to keep the corresponding named writeable registered so it can be deserialized properly).