packit / packit-service

Packit provided as a service
https://packit.dev
MIT License
37 stars 48 forks source link

Do we still need TaskResults? #1607

Open jpopelka opened 2 years ago

jpopelka commented 2 years ago

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.

TomasTomecek commented 2 years ago

Could celery monitoring use it?

jpopelka commented 2 years ago

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?

mfocko commented 3 months ago

Still relevant, some changes might have occurred

lbarcziova commented 2 months ago

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.