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

Do we still need TaskResults? #1607

Open
jpopelka opened this issue Aug 10, 2022 · 4 comments
Open

Do we still need TaskResults? #1607

jpopelka opened this issue Aug 10, 2022 · 4 comments
Labels
area/general Related to whole service, not a specific part/integration. complexity/single-task Regular task, should be done within days. gain/low This doesn't bring that much value to users. impact/low This issue impacts only a few users. kind/technical-debt The code needs love. Refactoring.

Comments

@jpopelka
Copy link
Member

TaskResults was introduced in #15 as HandlerResults because we wanted to somewhere store info about performed Celery tasks and this was the easiest since Celery can store task results (i.e. what a handler method returns) automatically once a backend (database) is configured.
It was before we started using PostgreSQL so the backend was the same as the broker, i.e. Redis.
After switching to PostgreSQL those results were stored there, but after some time we disabled it because we hadn't actually ever used/checked them.

The initial idea behind #15 was that HandlerResults.success would tell us whether all jobs succeeded and if not an Exception would be raised (sent to Sentry). And HandlerResults.details would contain more info from handlers, to be stored in the backend.

Given that we don't store the task results in the backend anymore, I'm wondering whether we actually use the TaskResults.details anywhere or whether it can be removed. And the same with TaskResults.success.

@jpopelka jpopelka changed the title Do we still need TaskResults.details? Do we still need TaskResults? Aug 10, 2022
@TomasTomecek
Copy link
Member

Could celery monitoring use it?

@jpopelka
Copy link
Member Author

I see the TaskResults.details being used in JobHandler.run_job() to log an error message in case of one of the jobs failed, but I guess the jobs can do the error logging themselves?

@lbarcziova lbarcziova added complexity/single-task Regular task, should be done within days. impact/low This issue impacts only a few users. area/general Related to whole service, not a specific part/integration. gain/low This doesn't bring that much value to users. kind/internal Doesn't affect users directly, may be e.g. infrastructure, DB related. kind/technical-debt The code needs love. Refactoring. and removed kind/internal Doesn't affect users directly, may be e.g. infrastructure, DB related. labels Apr 13, 2023
@mfocko
Copy link
Member

mfocko commented Aug 19, 2024

Still relevant, some changes might have occurred

@lbarcziova
Copy link
Member

When working on #2526 I removed the error logging of success=False tasks to reduce the number of events sent to Sentry. I initially wanted to remove TaskResults entirely, but I found it may be useful for testing purposes still.

@lbarcziova lbarcziova removed their assignment Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/general Related to whole service, not a specific part/integration. complexity/single-task Regular task, should be done within days. gain/low This doesn't bring that much value to users. impact/low This issue impacts only a few users. kind/technical-debt The code needs love. Refactoring.
Projects
Status: backlog
Development

No branches or pull requests

5 participants