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

81 supplier plugin health and store usage #84

Conversation

JohannesDaniel
Copy link
Collaborator

Description

The feature is expected to

  • evaluate the plugin health by checking the state of the circuit breaker
  • evaluate the plugin health by checking the health of the indices required for storing plugin-related objects
  • returning the plugin health using a traffic-light labeling (red, yellow, green)
  • gathering stats about stored features, featuresets and models

Issues Resolved

#81

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.

@JohannesDaniel
Copy link
Collaborator Author

@ylwu-amzn
@jngz-es

@ylwu-amzn
Copy link

Can you fix the failed CI ?

@sstults
Copy link
Collaborator

sstults commented Dec 12, 2024

Neither @JohannesDaniel nor I could reproduce the failure of JDK17. I reran the CI tests 10 times and still could not get a failure. Given that JDK17 is being phased out of main I'm going to merge this PR.

@sstults sstults merged commit e58e20d into opensearch-project:main Dec 12, 2024
5 checks passed
@JohannesDaniel
Copy link
Collaborator Author

@ylwu-amzn @sstults I think this is simply a test that fails from time to time




// private

Choose a reason for hiding this comment

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

remote this line ?

@@ -0,0 +1,61 @@
package org.opensearch.ltr.stats.suppliers;

Choose a reason for hiding this comment

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

Add license header

.reduce(STATUS_GREEN, this::combineStatuses);
}

private String combineStatuses(String s1, String s2) {

Choose a reason for hiding this comment

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

Make the variable name more readable: s1, s2

@ylwu-amzn
Copy link

Why this PR merged without approval?

@sstults
Copy link
Collaborator

sstults commented Dec 12, 2024

Why this PR merged without approval?

@ylwu-amzn that's me probably misunderstanding policy. My assumption was that by merging I'm implicitly approving. That was exacerbated by my other mistake of assuming that passing CI tests was your only comment on this PR.

@JohannesDaniel I'm going to revert so that the above issues can be resolved.

@ylwu-amzn
Copy link

ylwu-amzn commented Dec 13, 2024

@sstults no worry, I added more comments later. Thanks for the reverting

sstults pushed a commit to sstults/opensearch-learning-to-rank-base that referenced this pull request Dec 19, 2024
Signed-off-by: Johannes Peter <[email protected]>
(cherry picked from commit e58e20d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants