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

feat(job-runner): track job status #6332

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(job-runner): track job status #6332

wants to merge 1 commit into from

Conversation

xurui-c
Copy link
Member

@xurui-c xurui-c commented Sep 20, 2024

@xurui-c xurui-c changed the title c feat(job-runner): Implement a status subcommand for snuba jobs Sep 20, 2024
@xurui-c xurui-c changed the title feat(job-runner): Implement a status subcommand for snuba jobs feat(job-runner): track job status Sep 20, 2024
JOB_ID = "abc1234"
test_job_spec = JobSpec(job_id=JOB_ID, job_type="ToyJob")

# @pytest.fixture(autouse=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it turns out the state of the redis client does not persist outside, so after inserting ['abc1234', 'not started'] during init of ToyJob, redis_client.hkeys would show nothing inserted in test_job_status_changes_to_finished

Copy link

codecov bot commented Sep 20, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
579 1 578 1
View the top 1 failed tests by shortest run time
tests.cli.test_jobs test_cmd_line_valid
Stack Traces | 0.003s run time
Traceback (most recent call last):
  File ".../tests/cli/test_jobs.py", line 9, in test_cmd_line_valid
    result = runner.invoke(
             ^^^^^^^^^^^^^^
  File ".../local/lib/python3.11.../site-packages/click/testing.py", line 408, in invoke
    return_value = cli.main(args=args or (), prog_name=prog_name, **extra)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../local/lib/python3.11........./site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File ".../local/lib/python3.11........./site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../local/lib/python3.11........./site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/cli/jobs.py", line 50, in run
    job_to_run = JobLoader.get_job_instance(job_spec, dry_run)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/manual_jobs/job_loader.py", line 19, in get_job_instance
    return cast("Job", job_type_class(job_spec, dry_run=dry_run))
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/manual_jobs/toy_job.py", line 10, in __init__
    super().__init__(job_spec, dry_run)
  File ".../snuba/manual_jobs/__init__.py", line 34, in __init__
    self._set_status(NOT_STARTED)
  File ".../snuba/manual_jobs/__init__.py", line 54, in _set_status
    redis_client.hset(job_status_hash, self.job_spec.job_id, status)
  File ".../local/lib/python3.11.../redis/commands/core.py", line 4841, in hset
    return self.execute_command("HSET", name, *items)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/tests/conftest.py", line 97, in __call__
    pytest.fail(self.__message)
  File ".../local/lib/python3.11.../site-packages/_pytest/outcomes.py", line 196, in fail
    raise Failed(msg=reason, pytrace=pytrace)
Failed: attempted to access redis in test that does not use @pytest.mark.redis_db

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Comment on lines +37 to +39
self._set_status(RUNNING)
self._execute()
self._set_status(FINISHED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that we might want status tracking to live outside Job.I think of Job as more of an interface for users to implement, rather than something that contains logic for running jobs itself (I also think things can get confusing quickly when you rely on inheritance in Python).

What if we had something like a JobRunner static class or module that exposed utilities that would orchestrate the job run, and manage status based on the completely separate Job class? I think that produces a looser coupling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants