Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
andreidan
left a comment
There was a problem hiding this comment.
Thanks for refactoring this indicator Mary.
This is heading in the right direction 🚀
I left a few suggestions and questions
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
| .filter(routing -> indices.contains(routing.index().getName())) | ||
| .map(ShardRouting::currentNodeId) | ||
| .collect(Collectors.toSet()); | ||
| DiskHealthAnalyzer diskHealthAnalyzer = new DiskHealthAnalyzer(diskHealthInfoMap, blockedIndices, clusterState); |
There was a problem hiding this comment.
Since the DiskHealthAnalyzer already has the cluster state, could it look and compute the blocked indices?
We now compute them in the indicator but not use them anywhere else except passing them to DiskHealthAnalyzer.
What do you think?
There was a problem hiding this comment.
That is true, my reasoning was that the interface shows the information needed to calculate it, but we can document why we need the cluster state and leave it at that ;) .
There was a problem hiding this comment.
in that case I will move the details too.
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/HealthIndicatorDisplayValues.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public Index[] copyIndices() { | ||
| return shardsByIndex.keySet().toArray(Index.EMPTY_ARRAY); |
There was a problem hiding this comment.
@dakrone do you know of a different/more efficient way of getting all the indices on a node?
There was a problem hiding this comment.
@dakrone I merged this PR to move forward but please if you know about a better way to get all the indices per node, I will fix it in a new PR.
Co-authored-by: Andrei Dan <andrei.dan@elastic.co>
andreidan
left a comment
There was a problem hiding this comment.
Thanks for iterating on this Mary. LGTM 🚀
Left a suggestion
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/DiskHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
Introduced the DiskHealthAnalyzer which determines the health state, the symptom, the impacts, the diagnoses and the details. We also changed the retrieval of the indices per node to use the RoutingNodes instead of the RoutingTable directly for efficiency. Co-authored-by: Andrei Dan <andrei.dan@elastic.co>
💚 Backport successful
|
Introduced the DiskHealthAnalyzer which determines the health state, the symptom, the impacts, the diagnoses and the details. We also changed the retrieval of the indices per node to use the RoutingNodes instead of the RoutingTable directly for efficiency. Co-authored-by: Andrei Dan <andrei.dan@elastic.co>
During this refactoring we introduce the
DiskHealthAnalyzerwhich will determine the health state, the symptom, the impacts and the diagnoses.The benefits of this class is that the constructor is using the input information to fill in data structures that answer the questions we need to determine the symptom, the impacts and the diagnosis. These features are quite complex to calculate so the simpler the code there the best it is.
We also restructured the tests to say in the test name the expected status and then the premises, blocked indices and/or status of the nodes. We introduce a comment that explains what the tests simulates and what's expected.
Finally, we change the retrieval of the indices per node to use the
RoutingNodesinstead of theRoutingTabledirectly for efficiency.Closes #90212