-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[ML][Data Frame] add node attr to GET _stats #43842
[ML][Data Frame] add node attr to GET _stats #43842
Conversation
Pinging @elastic/ml-core |
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. left some minor comments
@@ -37,7 +38,9 @@ public void testFromXContent() throws IOException { | |||
DataFrameTransformStateTests::toXContent, | |||
DataFrameTransformState::fromXContent) | |||
.supportsUnknownFields(true) | |||
.randomFieldsExcludeFilter(field -> field.equals("current_position")) | |||
.randomFieldsExcludeFilter(field -> field.equals("current_position") || | |||
field.equals("node") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be simplified to field.startsWith("node")
but shouldn't it just be the map node.attributes
that is ignored.
@@ -64,7 +64,7 @@ protected boolean supportsUnknownFields() { | |||
|
|||
@Override | |||
protected Predicate<String> getRandomFieldsExcludeFilter() { | |||
return field -> field.equals("state.current_position"); | |||
return field -> field.equals("state.current_position") || field.equals("state.node") || field.equals("state.node.attributes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
node.getEphemeralId(), | ||
node.getAddress().toString(), | ||
// TODO add data_frame attributes when/if they are added | ||
Collections.emptyMap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make attributes
nullable so it is not written as an empty object attributes: {}
in toXContent
|
||
@Override | ||
protected Predicate<String> getRandomFieldsExcludeFilter() { | ||
return field -> !field.isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If another nested object besides the attribute map was added to NodeAttributes
it would be excluded here.
private static void setNodeAttributes(DataFrameTransformStateAndStats dataFrameTransformStateAndStats, | ||
PersistentTasksCustomMetaData persistentTasksCustomMetaData, | ||
ClusterState state) { | ||
var pTask = persistentTasksCustomMetaData.getTask(dataFrameTransformStateAndStats.getTransformId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var - how very modern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩
😄
@elasticmachine update branch |
jenkins retest this please |
run elasticsearch-ci/2 |
run elasticsearch-ci/packaging-sample |
1 similar comment
run elasticsearch-ci/packaging-sample |
* [ML][Data Frame] add node attr to GET _stats * addressing testing issues with node.attributes
This adds node information to the
_stats
call. I made thenode
field part of thestate
sub-object. This made the most sense to me asstate
also contains the task state information, and the reason for its current state.closes #43743