-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(tasks) Enable taskworkers by default in self-hosted #99374
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
Conversation
Change the default of `taskworker.enabled` to true which will enable taskworkers for self-hosted as well. Refs STREAM-450
| default=True, | ||
| flags=FLAG_AUTOMATOR_MODIFIABLE, |
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.
Potential bug: Changing the default of taskworker.enabled to True will break self-hosted deployments, which lack the required Kafka infrastructure and will fail to process background tasks.
-
Description: Changing the default for
taskworker.enabledtoTrueroutes background tasks to the taskworker system. This system relies on Kafka, as shown by topic definitions insrc/sentry/conf/types/kafka_definition.pyand producer logic insrc/sentry/taskworker/registry.py. Self-hosted deployments are configured with Redis/RabbitMQ for Celery and do not have Kafka infrastructure. The change lacks a corresponding override inself-hosted/sentry.conf.pyto keep taskworkers disabled. As a result, self-hosted instances will attempt to send tasks to non-existent Kafka brokers, causing background job processing to fail and breaking core Sentry functionality. -
Suggested fix: Add
SENTRY_OPTIONS["taskworker.enabled"] = Falsetoself-hosted/sentry.conf.py. This will maintain the existing Celery-based task processing for self-hosted instances and prevent breakages until they are prepared to migrate to the new taskworker system.
severity: 0.92, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
Along with getsentry/sentry#99374 and this change taskworkers will be enabled by default in self-hosted. I've left the celery workers active to smooth over any tasks that are in-flight during an upgrade. Add a worker healtcheck as we have one now. Refs STREAM-450
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #99374 +/- ##
==========================================
- Coverage 81.24% 81.19% -0.05%
==========================================
Files 8589 8591 +2
Lines 380056 380353 +297
Branches 24128 24128
==========================================
+ Hits 308760 308831 +71
- Misses 70943 71169 +226
Partials 353 353 |
| @patch("django.utils.timezone.now") | ||
| def test_basic_update(self, mock_timezone_now: MagicMock) -> None: | ||
| @freeze_time("2023-10-03 00:00:00") | ||
| def test_basic_update(self) -> None: |
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.
These tests were also duplicated in tests/sentry/api/endpoints/test_project_codeowners_details.py. It looks like the old test module was left behind during #97916
…heck (#3933) feat(tasks) Remove taskworker option override and add worker healthcheck Along with getsentry/sentry#99374 and this change taskworkers will be enabled by default in self-hosted. I've left the celery workers active to smooth over any tasks that are in-flight during an upgrade. Add a worker healtcheck as we have one now. Refs STREAM-450
Change the default of
taskworker.enabledto true which will enable taskworkers for self-hosted as well.Refs STREAM-450