Make cancel() synchronous in DBMAsyncJob #14717
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
edengorevoy
changed the title
edengorevoy
added
category/bugfix
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
justiniso
reviewed
Jun 8, 2023
justiniso
previously approved these changes
Jun 8, 2023
jmeunier28
reviewed
Jun 9, 2023
| @@ -809,9 +809,6 @@ def test_async_job_cancel(aggregator, dd_run_check, dbm_instance): | |||
| mysql_check = MySql(common.CHECK_NAME, {}, [dbm_instance]) | |||
| dd_run_check(mysql_check) | |||
| mysql_check.cancel() | |||
| # wait for it to stop and make sure it doesn't throw any exceptions | |||
| mysql_check._statement_samples._job_loop_future.result() | |||
| mysql_check._statement_metrics._job_loop_future.result() | |||
jmeunier28
approved these changes
Jun 9, 2023
| self._cancel_event.set() | ||
|
|
||
| if self._job_loop_future is not None: | ||
| self._job_loop_future.result(10) | ||
|
|
There was a problem hiding this comment.
Hey @jmeunier28 @edengorevoy 👋 Keep in mind that you'll have to bump the minimum base package version in your integrations if they now rely on this change 🙂 (e.g. https://github.com/DataDog/integrations-core/blob/master/mysql/pyproject.toml#L33)
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR adds some logic in the
cancel()method ofDMBAsyncJobto ensure that cancel() is a synchronous operation.This PR also edits some tests in the postgres, MySQL, and sqlserver agent integrations (those who inherit from
DMBAsyncJob) to try to make them less flaky. The change adds arun_one_checkfunction in the postgres test utils to run a check and then cancel it + wait for all threads to cancel, so that assertions on metrics happen in a single-threaded environment.Motivation
We are running into some testing issues in this PR. The errors are potentially race conditions that arise at runtime due to the metrics list in the Aggregator being iterated over at the same time as they're being added to in separate threads owned by DBM.
We should make sure cancellation is synchronous in DBMAsyncJob to further debug this issue and verify that it is the source of these errors.
DBMAsyncJob is only used by objects owned by DBM
, so making this change should not cause issues in other teams.
Additional Notes
I intend to rebase the above PR on this branch so I can see how changing the condition for starting the job loop affects the tests, and I want to use this modified cancel() function to facilitate that investigation.
Review checklist (to be filled by reviewers)
changelog/andintegration/labels attachedqa/skip-qalabel.