Extend repository_integrity health indicator for unknown and invalid repos
#104614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nielsbauman
added
>enhancement
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @nielsbauman, I've created a changelog YAML for you. |
nielsbauman
commented
Jan 22, 2024
gmarouli
reviewed
Jan 30, 2024
server/src/main/java/org/elasticsearch/health/node/LocalHealthMonitor.java
Outdated
Show resolved
Hide resolved
gmarouli
reviewed
Jan 30, 2024
server/src/main/java/org/elasticsearch/health/node/LocalHealthMonitor.java
Outdated
Show resolved
Hide resolved
gmarouli
requested changes
Jan 30, 2024
There was a problem hiding this comment.
Thank you @nielsbauman for picking this up. This is a great start, I focused my comments at 2 points that I found a bit more difficult to follow.
gmarouli
requested changes
Jan 31, 2024
There was a problem hiding this comment.
Thanks for working on this @nielsbauman . I haven't finished the review but because you have been waiting long, I send this initial comments and I will continue tomorrow.
server/src/main/java/org/elasticsearch/health/node/HealthInfo.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/RepositoriesHealthInfo.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/UpdateHealthInfoCacheAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
gmarouli
requested changes
Feb 2, 2024
There was a problem hiding this comment.
Hey @nielsbauman great progress, I think the structure is already in place and now we are moving towards the small changes.
I moved further with the review; however I would like to ask you if you can revert the conversion of assertThat to assertEquals, since the deprecation is fixed and the code is already there it will greatly help the review process. Changing these assertions introduces extra noise in the PR and it's harder to find the actual changes.
Thank you for the great work!
In the new tests you write you can use the assertion you prefer, no problem there.
docs/reference/troubleshooting/snapshot/add-repository.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/troubleshooting/snapshot/add-repository.asciidoc
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/HealthInfoCache.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/LocalHealthMonitor.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/health/node/LocalHealthMonitor.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/node/tracker/RepositoriesHealthTrackerTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/node/FetchHealthInfoCacheActionTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/node/FetchHealthInfoCacheActionTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/health/node/FetchHealthInfoCacheActionTests.java
Outdated
Show resolved
Hide resolved
gmarouli
reviewed
Feb 6, 2024
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Show resolved
Hide resolved
gmarouli
reviewed
Feb 6, 2024
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
gmarouli
reviewed
Feb 6, 2024
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Show resolved
Hide resolved
gmarouli
reviewed
Feb 6, 2024
server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java
Outdated
Show resolved
Hide resolved
gmarouli
requested changes
Feb 6, 2024
# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
gmarouli
approved these changes
Feb 7, 2024
There was a problem hiding this comment.
LGTM, assuming the CI agrees too :) . Congrats on completing an important and complex feature @nielsbauman !!!
nielsbauman
added a commit
that referenced
this pull request
Feb 19, 2024
nielsbauman
added a commit
to nielsbauman/elasticsearch
that referenced
this pull request
Feb 19, 2024
nielsbauman
added a commit
that referenced
this pull request
Feb 19, 2024
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 18, 2024
This page was split up in elastic#104614 but the `ReferenceDocs` symbol links to the top-level page still rather than the correct subpage. This fixes the link.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 18, 2024
Since elastic#104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
DaveCTurner
added a commit
that referenced
this pull request
Apr 18, 2024
DaveCTurner
added a commit
that referenced
this pull request
Apr 18, 2024
Since #104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 18, 2024
This page was split up in elastic#104614 but the `ReferenceDocs` symbol links to the top-level page still rather than the correct subpage. This fixes the link.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 18, 2024
Since elastic#104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 18, 2024
Since elastic#104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 18, 2024
Since #104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 18, 2024
Since #104614 the top-level repo troubleshooting page is just a short paragraph which talks about "this page" but in fact refers to information spread across a number of subsequent pages. It's not obvious to the reader that they need to use the navigation menu to get to the information they seek. Moreover we link to this page from an exception message today so there's a reasonable chance that users will find it when trying to troubleshoot a genuine problem. This commit rewords things slightly and adds links to the subsequent pages to the body of the page to avoid this confusion.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main objective of this PR is to address the linked issue below. The main changes in this PR include:
LocalHealthMonitor: to allow for a more "scalable" setup, I added the abstract classHealthCheck<T>, which represents a health check that will be performed on every node, and madeLocalHealthMonitorwork with a list ofHealthChecks. This reduces future effort to add new health checks.DiskCheckto a separate class/file.RepositoriesCheckfor this new feature.RepositoryIntegrityHealthIndicatorService: refactored this class to do a more extensive analysis of the repository health in the cluster.TransportVersion, constructor updates, etc.Fixes #103784