-
Notifications
You must be signed in to change notification settings - Fork 517
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
Support for snuba additional arguments, livenessProbe & readinessProbe #1078
base: develop
Are you sure you want to change the base?
Conversation
…consumer and subscriptions-scheduler-executor
I think it’s worth adding default probes for consumers, which will be enabled by default and leave it possible to overwrite them in the values file |
@adonskoy in order to have a valid probe for consumers the arroyo HealthCheck strategy needs to be instantiated. So therefore both of the values that I provided ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change additionArgs as i mentioned
@@ -655,6 +656,20 @@ snuba: | |||
# maxBatchTimeMs: "" | |||
# queuedMaxMessagesKbytes: "" | |||
# queuedMinMessages: "" | |||
additionalArgs: [] | |||
# additionalArgs: | |||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pyhp2017 if I understand correctly you want me to remove the additionalArgs
array and instead have a plain string value like:
healthCheckFilePath: "/tmp/health.txt"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the commented area you have this: "--health-check-file=/tmp/health.txt"
which is not correct:
--healthcheck-file-path TEXT A file to touch roughly every second to indicate that the
consumer is still alive. See
https://getsentry.github.io/arroyo/strategies/healthcheck.html
for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pyhp2017 I am invoking the snuba consumer
& snuba subscriptions-scheduler-executor
cli commands.
Both commands accept an argument named --health-check-file
:
which maps the underlying arroyo healthcheck strategy.
I have also tested the above in my personal kubernetes cluster and it works as --health-check-file
.
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
@@ -736,6 +777,20 @@ snuba: | |||
autoOffsetReset: "earliest" | |||
# volumes: [] | |||
# volumeMounts: [] | |||
additionalArgs: [] | |||
# additionalArgs: | |||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
@@ -750,6 +805,21 @@ snuba: | |||
autoOffsetReset: "earliest" | |||
# volumes: [] | |||
# volumeMounts: [] | |||
additionalArgs: [] | |||
# additionalArgs: | |||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
@@ -900,6 +1039,21 @@ snuba: | |||
# queuedMaxMessagesKbytes: "" | |||
# queuedMinMessages: "" | |||
|
|||
additionalArgs: [] | |||
# additionalArgs: | |||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
|
||
additionalArgs: [] | ||
# additionalArgs: | ||
# - --health-check-file=/tmp/health.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to: "--healthcheck-file-path"
…consumer and subscriptions-scheduler-executor
# Conflicts: # sentry/Chart.yaml
@pyhp2017 any update on this? |
👋 Hi, @artemistomaras, |
This pull request references issue #1076