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

Refactor Handlers for None-correctness #1581

Closed
FrNecas opened this issue Jul 14, 2022 · 2 comments · Fixed by #1678
Closed

Refactor Handlers for None-correctness #1581

FrNecas opened this issue Jul 14, 2022 · 2 comments · Fixed by #1678
Assignees
Labels
kind/technical-debt The code needs love. Refactoring.

Comments

@FrNecas
Copy link

FrNecas commented Jul 14, 2022

Follow up of the discussion with @lbarcziova in #1579 (comment)

Currently, handlers are instantiated in 2 times, once in Steve and then in the celery task corresponding to the handler. However, the use-cases for these two initializations are different:

  • Steve just uses pre_check() and perhaps some other stuff but doesn't run it
  • celery task runs the handler

This causes problems with regards to None correctness as explained in the comment linked above. Ideally, a task that can be retried should not have its CeleryTask argument set to None (without the task attached, retrying is not possible and does not make sense). However, the initialization inside Steve does not have a celery task yet to attach, hence it must be set to None (a bit of a chicken-egg problem).

A solution that I assume could work (however, feel free to come up with anything else):

  • split each handler (including the abstract classes) into 2 parts (naming is just a suggestion):
    • BaseHandler - implements just the pre_check and similar stuff
    • RunnableHandler - also implements run and inherits from BaseHandler
  • inside Steve initialize just the BaseHandler which does not need a CeleryTask even for a retriable job handler.
  • inside tasks, initialize RunnableHandler which in the case of retriable handlers will have non-Optional CeleryTask argument

This is a blocker for #1433 (the implementation from #1579 would not pass mypy's strict checks -- however it does not make sense to block it on this rather large refactoring)

@FrNecas FrNecas added the kind/technical-debt The code needs love. Refactoring. label Jul 14, 2022
@FrNecas
Copy link
Author

FrNecas commented Jul 21, 2022

Follow-up from arch:

Another option to handle this situation is to not instantiate a handler in Steve. This would mean that pre_check and check_if_actor_can_run_job_and_report would have to be made static/class methods and everything required would have to be passed to them. From briefly looking at this, the methods access all kinds of stuff from the instance attributes (job/package config, helper instance -- this would have to be instantiated, which requires ogr Project, trigger and a lot of other stuff) so this approach may not really be feasible, the code of these methods may get quite messy, if we wanted to pass everything there.

@stale

This comment was marked as resolved.

@stale stale bot added the stale Is the issue still valid? label Sep 21, 2022
@lachmanfrantisek lachmanfrantisek removed the stale Is the issue still valid? label Sep 22, 2022
@majamassarini majamassarini moved this from refined [10] to in-progress [10] in Packit Kanban Board Sep 23, 2022
@majamassarini majamassarini self-assigned this Sep 23, 2022
@majamassarini majamassarini moved this from in-progress [10] to in-review in Packit Kanban Board Sep 29, 2022
@majamassarini majamassarini moved this from in-review to done in Packit Kanban Board Oct 25, 2022
@majamassarini majamassarini moved this from done to ready-for-release in Packit Kanban Board Oct 25, 2022
softwarefactory-project-zuul bot added a commit that referenced this issue Oct 25, 2022
Move job handler's pre-checks in a new class

Job handlers should be retriable and may have a CeleryTask reference.
If a job handler is instantiated just for calling the pre-check method
then it can not be given a CeleryTask reference.
The pre_check method is now a classmethod for any job handler.
The job handler's pre_check method will invoke the pre_check method of
a list of new checker classes.
A checker class needs all the parameters needed by a job handler class
but it does not need the CeleryTask reference.
All the common code between checkers and handlers has been moved
in mixin classes.
Fixes #1581

Reviewed-by: None <None>
Reviewed-by: František Nečas <[email protected]>
Reviewed-by: Maja Massarini <None>
Reviewed-by: Laura Barcziová <None>
Reviewed-by: Matej Focko <None>
Repository owner moved this from ready-for-release to done in Packit Kanban Board Oct 25, 2022
@majamassarini majamassarini moved this from done to ready-for-release in Packit Kanban Board Oct 26, 2022
@majamassarini majamassarini moved this from ready-for-release to done in Packit Kanban Board Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/technical-debt The code needs love. Refactoring.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants