Skip to content

Conversation

@benwtrent
Copy link
Member

@benwtrent benwtrent commented Sep 19, 2018

Non-intrusive way to add node_count for the ML telemetry.

Closes #33850

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one idea.

Also, if you find that the PR build fails in the BWC tests because an ML usage object is getting serialized on the wire between 6.5 and 7.0 before your backport, the solution is to change the BWC constants from 6.5.0 to 7.0.0 in this PR, then change back to 6.5.0 when you backport to 6.x, and finally push another commit to master setting master to 6.5.0 again.

this.datafeedsUsage = in.readMap();
if (in.getVersion().onOrAfter(Version.V_6_5_0)) {
this.nodeCount = in.readInt();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than use 0 in the case of "unknown" in a mixed version cluster it would be nicer to have a specific "unknown" value, say -1, and then not include the count in the JSON output if it's unknown.

For example:

        if (in.getVersion().onOrAfter(Version.V_6_5_0)) {
            this.nodeCount = in.readInt();
        } else {
            this.nodeCount = -1;
        }


private final Map<String, Object> jobsUsage;
private final Map<String, Object> datafeedsUsage;
private int nodeCount;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be final too

if (datafeedsUsage != null) {
builder.field(DATAFEEDS_FIELD, datafeedsUsage);
}
builder.field(NODE_COUNT, nodeCount);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going with the approach of not printing "unknown", this can be:

        if (nodeCount >= 0) {
            builder.field(NODE_COUNT, nodeCount);
        }

Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benwtrent benwtrent merged commit 4767a01 into elastic:master Sep 19, 2018
@benwtrent benwtrent deleted the feature/num_of_ml_nodes_statistics branch September 19, 2018 20:35
codebrain added a commit to elastic/elasticsearch-net that referenced this pull request Mar 14, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 18, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 20, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 21, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Apr 3, 2019
Relates: elastic/elasticsearch#33863
Add usage remarks.

(cherry picked from commit 8b31b27)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants