Add dynamic setting for DiskCircuitBreaker default disk shortage thre…

[pranavjad]

Description

Adds a dynamic setting for the disk shortage threshold in the DiskCircuitBreaker.

Issues Resolved

#1341

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[ylwu-amzn]

Thanks for making this change. The CI failed, check https://github.com/opensearch-project/ml-commons/actions/runs/6592907739/job/17914444093?pr=1532

* What went wrong:
Execution failed for task ':opensearch-ml-plugin:spotlessJavaCheck'.
> The following files had format violations:
      src/main/java/org/opensearch/ml/breaker/DiskCircuitBreaker.java
          @@ -11,9 +11,9 @@
           import·java.security.AccessController;
           import·java.security.PrivilegedActionException;
           import·java.security.PrivilegedExceptionAction;
          +
          +import·org.opensearch.cluster.service.ClusterService;
           import·org.opensearch.common.settings.Settings;
          -import·org.opensearch.cluster.service.ClusterService;
          -
           import·org.opensearch.ml.common.exception.MLException;
           
           /**
          @@ -36,6 +36,7 @@
           ········super(threshold);
           ········this.diskDir·=·diskDir;
           ····}
          +
           ····public·DiskCircuitBreaker(Settings·settings,·ClusterService·clusterService,·String·diskDir)·{
           ········super(DEFAULT_DISK_SHORTAGE_THRESHOLD);
           ········this.diskDir·=·diskDir;
      src/main/java/org/opensearch/ml/settings/MLCommonsSettings.java
          @@ -169,14 +169,7 @@
           ········);
           
           ····public·static·final·Setting<Long>·ML_COMMONS_DISK_SHORTAGE_THRESHOLD·=·Setting
          -············.longSetting(
          -················"plugins.ml_commons.disk_shortage_threshold",
          -················5L,
          -················1L,
          -················10L,
          -················Setting.Property.NodeScope,
          -················Setting.Property.Dynamic
          -············);
          +········.longSetting("plugins.ml_commons.disk_shortage_threshold",·5L,·1L,·10L,·Setting.Property.NodeScope,·Setting.Property.Dynamic);
           ····public·static·final·Setting<Boolean>·ML_COMMONS_MEMORY_FEATURE_ENABLED·=·ConversationalIndexConstants.ML_COMMONS_MEMORY_FEATURE_ENABLED;
           
           ····//·Feature·flag·for·enabling·search·processors·for·Retrieval·Augmented·Generation·using·OpenSearch·and·Remote·Inference.
  Run './gradlew :opensearch-ml-plugin:spotlessApply' to fix these violations.

[pranavjad]

The integTest seems to be failing, not sure whats going on here.

[ylwu-amzn]

Seems flaky as some platform succeed , some failed, rerun the failed test.

[austintlee]

Can you add some tests for this?

[pranavjad]

Is there any reference for adding tests I can look at? I'm not really sure where to add tests or what to test.

[dhrubo-os]

Is there any reference for adding tests I can look at? I'm not really sure where to add tests or what to test.

Could you please check how other variables were tested? For example ML_COMMONS_MEMORY_FEATURE_ENABLED ?

This will help you to deep dive in the code.

[dhrubo-os]

@pranavjad do you understand now how to add tests? Please let us know if you are still unclear about this.

[pranavjad]

@dhrubo-os I looked through the test folder under plugin and saw how the tests for MemoryCircuitBreaker and MLCircuitBreaker were made, but when creating a DiskCircuitBreaker object in a test, what should I use as the diskDir argument?

[dhrubo-os]

@dhrubo-os I looked through the test folder under plugin and saw how the tests for MemoryCircuitBreaker and MLCircuitBreaker were made, but when creating a DiskCircuitBreaker object in a test, what should I use as the diskDir argument?

@Zhangxunmt Looks like you worked in this class. Could you please help @pranavjad? Thanks.

[Zhangxunmt]

Again, can we add one more test case covering this change?

[Zhangxunmt]

https://github.com/opensearch-project/ml-commons/blob/2.x/plugin/src/main/java/org/opensearch/ml/plugin/MachineLearningPlugin.java#L329, Please use this Dir for your tests. @pranavjad

[ylwu-amzn]

How about rename the setting name?

Suggested change

	.longSetting("plugins.ml_commons.disk_shortage_threshold", 5L, 1L, 10L, Setting.Property.NodeScope, Setting.Property.Dynamic);
	.longSetting("plugins.ml_commons.disk_space_threshold", 5L, 1L, 10L, Setting.Property.NodeScope, Setting.Property.Dynamic);

[dhrubo-os]

@pranavjad are you working on this? Can we please wrap up this PR? Please let me know if you need any help.

[dhrubo-os]

@pranavjad are you working on this?

[pranavjad]

No, I am not sorry. I do not think I will be able to finish this PR, so I will close it. Thanks.