-
-
Notifications
You must be signed in to change notification settings - Fork 56
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): async job functionality #6427
base: master
Are you sure you want to change the base?
Conversation
159cde0
to
61142e8
Compare
statuses = _get_job_status_multi( | ||
[_build_job_status_key(job_id) for job_id in job_ids] | ||
) | ||
for i in range(len(job_ids)): |
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.
I know this for loop defeats the purpose of mget used in _get_job_status_multi
, so I'm also not opposed to write _get_is_job_async_multi
and _get_job_types_multi
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.
What would the implementations of _get_is_job_async_multi
and _get_job_types_multi
look like?
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.
We got rid of [get_is_job_async](https://github.com/getsentry/snuba/pull/6427#discussion_r1809149842)
, so I won't be implementing _get_is_job_async_multi
. The implementation of _get_job_types_multi
will just mirror _get_job_types_multi
and _get_job_status_multi
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.
^ will revisit this in subsequent PR
61142e8
to
1619b2e
Compare
3b11497
to
641d35a
Compare
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.
I think the overall logic is fine but we need a test that triggers the transition of async job from ASYNC_RUNNING_BACKGROUND
to FINISHED
.
641d35a
to
47b762c
Compare
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
def test_job_status_changes_from_async_running_background_to_finished() -> None: | ||
assert get_job_status(JOB_ID) == JobStatus.NOT_STARTED | ||
run_job(async_job_spec) | ||
with patch.object(AsyncJob, "async_finished_check", return_value=True): | ||
assert get_job_status(JOB_ID) == JobStatus.FINISHED |
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.
sorry to be annoying, but could we add a third assertion here to verify this job passes through ASYNC_RUNNING_BACKGROUND
before reaching FINISHED
?
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.
Looks fine if the test faillure is just a flake
47b762c
to
b98314f
Compare
https://getsentry.atlassian.net/jira/software/c/projects/SNS/boards/147?selectedIssue=SNS-2871
At times we will want to execute long-running ClickHouse jobs that don’t fit neatly into the migrations system; either they only run in a single environment, or we don’t want to block later migrations on them finishing. This PR supports users to create job types that can run asynchronously.