-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Monitoring] Use 0 as the default for shard count if the node is not found #21000
[Monitoring] Use 0 as the default for shard count if the node is not found #21000
Conversation
@@ -58,6 +58,8 @@ export function getShardStats(req, esIndexPattern, cluster, { includeNodes = fal | |||
} | |||
}; | |||
|
|||
console.log('CHRIS', JSON.stringify(params)); | |||
|
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.
please remove :D
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.
haha oh wow I'm ashamed that I left that in!
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.
Eh, we've all done it. #21002 :D
@@ -35,7 +35,7 @@ export function mapNodesInfo(nodeHits, clusterStats, shardStats) { | |||
isOnline, | |||
nodeTypeLabel: nodeTypeLabel, | |||
nodeTypeClass: nodeTypeClass, | |||
shardCount: get(shardStats, `nodes[${sourceNode.uuid}].shardCount`, null), | |||
shardCount: get(shardStats, `nodes[${sourceNode.uuid}].shardCount`, 0), |
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.
👍 It looks good to fix the bug this way. It will help the UI make sense of the data and be able to sort it correctly when the table is sorted by number of shards.
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
Looks like a flaky test:
jenkins, test this |
💔 Build Failed |
jenkins, test this |
💔 Build Failed |
You might want to try rebasing onto the latest master, @chrisronline. |
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 pending passing tests of course. I agree with Tim that this is a better solution than null
.
💚 Build Succeeded |
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!
I've seen an issue where this bug was reported in 5.5.3. I don't think this fix will be cleanly backportable that far. Feel free to backport this as far back as it will cleanly merge. |
…found (elastic#21000) * Use 0 as the default for shard count if the node is not found * Remove debug * Updating snapshot tests * Update api integration test
…found (elastic#21000) * Use 0 as the default for shard count if the node is not found * Remove debug * Updating snapshot tests * Update api integration test
@tsullivan It only goes back to 6.3 cleanly so will backport as far as there |
This didn't make it to 6.4.0 v. |
…found (elastic#21000) * Use 0 as the default for shard count if the node is not found * Remove debug * Updating snapshot tests * Update api integration test
Fixes #20628
This PR changes the default for
shardCount
fromnull
to0
.@tsullivan Is this the right way to fix it? Or is the real problem that the coordinating node isn't returned in the
getShardStats()
call?