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: Add task abortion test #11881

Closed
wants to merge 1 commit into from

Conversation

zation99
Copy link
Contributor

@zation99 zation99 commented Dec 16, 2024

Summary: A new thread to randomly tasks. It uses flag task_abort_interval_ms to control the frequency.

Differential Revision: D67267972

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4a959d2
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/676496b6917d740008cee268

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zation99 LGTM % minors. Please also have an review from @tanjialiang Thanks!

velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
@zation99 zation99 changed the title Add task abortion test feat: Add task abortion test Dec 16, 2024
zation99 added a commit to zation99/velox that referenced this pull request Dec 17, 2024
Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

zation99 added a commit to zation99/velox that referenced this pull request Dec 17, 2024
Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

zation99 added a commit to zation99/velox that referenced this pull request Dec 18, 2024
Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

Copy link
Contributor

@tanjialiang tanjialiang left a comment

Choose a reason for hiding this comment

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

Thanks @zation99. Added some comments. Also could you please help to cleanup the no longer needed code from #11506 since your PR changes the way of aborting.

velox/exec/fuzzer/MemoryArbitrationFuzzer.cpp Outdated Show resolved Hide resolved
Comment on lines 856 to 871
if (FLAGS_task_abort_interval_ms == 0) {
return;
}
while (!stop) {
std::this_thread::sleep_for(
std::chrono::milliseconds(FLAGS_task_abort_interval_ms));
auto tasksList = Task::getRunningTasks();
auto index = getRandomIndex(rng_, tasksList.size() - 1);
++taskAbortRequestCount;
tasksList[index]->requestAbort();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we have error handling in this thread, by having a try and catch all block

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we can consider to use folly:FunctionScheduler to achieve the periodic scheduling of this task. It provides better functionality and encapsulations.

zation99 added a commit to zation99/velox that referenced this pull request Dec 19, 2024
Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

zation99 added a commit to zation99/velox that referenced this pull request Dec 19, 2024
Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

Summary:

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67267972

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in b4a7479.

athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
Summary:
Pull Request resolved: facebookincubator#11881

A new thread to randomly tasks. It uses flag seconds_to_abort to control the frequency.

Reviewed By: xiaoxmeng

Differential Revision: D67267972

fbshipit-source-id: d61cb7704a85b5ad762f550aa622acbd26700501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants