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

fix: readiness probe on historical #98

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

cyril-corbon
Copy link
Collaborator

@cyril-corbon cyril-corbon commented Aug 22, 2023

Fixes #97

Description

  1. change historical readiness path to /druid/historical/v1/readiness
    Both path has similar purpose. https://druid.apache.org/docs/latest/operations/api-reference/#historical
Similar to /druid/historical/v1/loadstatus, but instead of returning JSON with a flag, responses 200 OK if segments in the local cache have been loaded, and 503 SERVICE UNAVAILABLE, if they haven't.
  1. add defaultProbes bool in druid spec to true by default
  2. update docs and document the issue with middle manager less until we add test case and a fix

This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • controllers/druid/handler.go
  • docs/features.md
  • chart/templates/crds/druid.apache.org_druids.yaml
  • config/crd/bases/druid.apache.org_druids.yaml

@cyril-corbon cyril-corbon force-pushed the fix/historical-probe branch 2 times, most recently from ef25a9d to 5e33880 Compare August 22, 2023 06:53
itamar-marom
itamar-marom previously approved these changes Aug 22, 2023
@cyril-corbon cyril-corbon dismissed itamar-marom’s stale review August 22, 2023 20:55

pr has been modified please take another look

// DefaultProbes If set to true this will add default probes (liveness / readiness / startup) for all druid components
// but it won't override existing probes
// +optional
// +kubebuilder:default:=false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be the default unless people want to disable it. Because it should work best "out of the box"

@@ -1143,7 +1143,7 @@ func setLivenessProbe(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) *v1.P
livenessProbe := updateDefaultPortInProbe(
firstNonNilValue(nodeSpec.LivenessProbe, m.Spec.LivenessProbe).(*v1.Probe),
nodeSpec.DruidPort)
if livenessProbe == nil {
if livenessProbe == nil && m.Spec.DefaultProbes == true {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need the "== true"
also on the below ones

Signed-off-by: Cyril Corbon <corboncyril@gmail.com>
Copy link
Collaborator

@itamar-marom itamar-marom left a comment

Choose a reason for hiding this comment

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

Did you use the make api-docs command for the API generation? If so great. If not, you should use it to generate the API specification.
Approved anyway :)

@cyril-corbon
Copy link
Collaborator Author

Did you use the make api-docs command for the API generation? If so great. If not, you should use it to generate the API specification. Approved anyway :)

yes I used make api-docs

@cyril-corbon cyril-corbon merged commit 386ac8a into datainfrahq:master Aug 23, 2023
@cyril-corbon cyril-corbon deleted the fix/historical-probe branch August 23, 2023 14:07
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.

Default readiness on historicals breaks operator.
2 participants