Skip to content

Conversation

@carols10cents
Copy link
Contributor

@carols10cents carols10cents commented Feb 21, 2025

Which issue does this PR close?

Rationale for this change

The behavior observed in #14036 was hard to reproduce and quantify; having a benchmark makes that easier!

What changes are included in this PR?

This benchmark loads multiple files into an in-memory object store, starts a datafusion query in a new tokio runtime, lets the query run for an amount of time, cancels the query, and measures how long it takes to drop the tokio runtime.

This demonstrates datafusion is likely not yielding often enough to allow for timely query cancellation and freeing up of all resources.

Are these changes tested?

This PR is only tests :)

Are there any user-facing changes?

Nope!

@carols10cents
Copy link
Contributor Author

Let's see if this works too....

/benchmark

Connects to apache#14036.

This benchmark loads multiple files into an in-memory object store,
starts a datafusion query in a new tokio runtime, lets the query run for
an amount of time, cancels the query, and measures how long it takes to
drop the tokio runtime.

This demonstrates datafusion is likely not yielding often enough to
allow for timely query cancellation and freeing up of all resources.
@alamb
Copy link
Contributor

alamb commented Feb 24, 2025

Let's see if this works too....

/benchmark

It doesn't sadly -- we removed this due to potential security concerns

@carols10cents
Copy link
Contributor Author

Let's see if this works too....
/benchmark

It doesn't sadly -- we removed this due to potential security concerns

Ok-- I'll remove the documentation about it in my benchmark docs PR.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @carols10cents

I tested this PR with

./benchmarks/bench.sh run cancellation

And it looked like this (👍 )

benchmarks/data/cancellation -o /Users/andrewlamb/Software/datafusion/benchmarks/results/cancellation-test-case/cancellation.json`
No data files found, generating (this will take a bit)
Generating file 1 of 7
Generating file 2 of 7
Generating file 3 of 7
Generating file 4 of 7
Generating file 5 of 7
Generating file 6 of 7
Generating file 7 of 7
Done generating files
Using 7 files now on disk
Starting to load data into in-memory object store
Done loading data into in-memory object store
in main, sleeping
Starting spawned
Creating logical plan...
Creating physical plan...
Executing physical plan...
Getting results...
cancelling thread
done dropping runtime in 32.72425ms
Iteration 0 cancelled in 32.724250000000005 ms
in main, sleeping
...

I made a small comment suggestion, but I think we can do it as a follow on PR


/// Test performance of cancelling queries
///
/// The queries are executed on a synthetic dataset generated during
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The queries are executed on a synthetic dataset generated during
/// Queries in DataFusion should stop executing "quickly" after they are
/// cancelled (the output stream is dropped).
///
/// The queries are executed on a synthetic dataset generated during

@alamb alamb merged commit 21fa511 into apache:main Feb 26, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 26, 2025

Thanks again @carols10cents

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