Skip to content
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

Add support for multiple paths in DiskSpaceHealthIndicator #27663

Closed
wants to merge 2 commits into from
Closed

Add support for multiple paths in DiskSpaceHealthIndicator #27663

wants to merge 2 commits into from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Aug 16, 2021

Fixed gh-18359

Assumption

I know there was a previous proposal that was closed due to concerns around breaking compatibility of the actuator output when configuring each path's threshold. This code proposal assumes that breaking the output is ok since this is going into 2.6.

Etiquette

I am not sure what the etiquette is in the situation where there is a dated closed proposal. I am starting to work on the counterpart of this for the DiskSpaceMetrics and figured I would put this together. If instead, we should resurrect the elder proposal I am totally fine w/ that as well. This newer implementation does support configuring the threshold per each path. I am not trying to be rude or step on toes.

Usage

The config props now look like the following:

management.health.diskspace.paths:

  - path: /some/dir/path1
    threshold: 10GB

  - path: /some/other/path2
  
  - path: /some/extra/path3
    threshold: 100MB
  • I am not super excited about the "paths.path"
  • With low effort we could also keep the existing "management.health.diskspace.path" for the singular case. Thoughts?

*/
private File path = new File(".");
private List<PathInfo> paths = Arrays.asList(new PathInfo());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As stated in the overview comment, we could keep support of the management.health.diskspace.path (for singular case only) by mapping a private PathInfo path = new PathInfo(); here as well.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 16, 2021
@@ -71,4 +99,13 @@ void runWhenDisabledShouldNotCreateIndicator() {
.run((context) -> assertThat(context).doesNotHaveBean(DiskSpaceHealthIndicator.class));
}

@SafeVarargs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yuk....

Alternative is to use List<Map.Entry<K, V> entries> which then leads to the tests having to use something like Arrays.asList(...). I went w/ convenience of callers namely because this is test code.

@wilkinsona
Copy link
Member

wilkinsona commented Aug 16, 2021

This code proposal assumes that breaking the output is ok since this is going into 2.6

The breaking changes we rejected previously, not because of their timing, but because there hadn't been much demand for them. As such, it was hard to justify the cost of the breaking change. The situation has changed a little bit since then as the request for disk space metrics is related to this and I think it makes sense for the two to be in sync in terms of their configuration. I'm not sure if that warrants making a breaking change in 2.6, if we should defer everything till 3.0, or if we should ship disk space metrics with configuration that's out of sync with disk space health. Flagging for team meeting so that we can discuss our options once again.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 16, 2021
@wilkinsona
Copy link
Member

Thanks for the proposal here, @bono007. In light of #27306 (comment), we no longer want to add support for configuring the disk space health indicator with multiple paths.

@wilkinsona wilkinsona closed this Aug 23, 2021
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add multiple paths to disk space health check
3 participants