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.

Sign up for GitHub

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

Jump to bottom

[C++] Clean interruption of a Acero plan with use_threads=false #35935

Closed
rtpsw opened this issue Jun 6, 2023 · 1 comment · Fixed by #35953
Closed

[C++] Clean interruption of a Acero plan with use_threads=false #35935

rtpsw opened this issue Jun 6, 2023 · 1 comment · Fixed by #35953
Assignees
@rtpsw
Labels
Component: C++ Type: enhancement
Milestone
13.0.0

Comments

@rtpsw
Copy link
Contributor

rtpsw commented Jun 6, 2023

Describe the enhancement requested

In internal experiments, we observed that an Acero plan with use_threads=false may not fully respond to an interruption. In this case, even though the signal is handled, the source nodes continue to push batches until done. Part of this issue is to devise testing for this condition.

Component(s)

C++
@rtpsw
Copy link
Contributor Author

rtpsw commented Jun 6, 2023

take

rtpsw added a commit to rtpsw/arrow that referenced this issue Jun 6, 2023
@rtpsw
apacheGH-35935: [C++] Clean interruption of a Acero plan with `use_th…
3317f94 
…reads=false`
@github-actions github-actions bot mentioned this issue Jun 6, 2023
Merged
rtpsw added a commit to rtpsw/arrow that referenced this issue Jun 6, 2023
@rtpsw
Merge branch 'main' into apacheGH-35935
bc0b941
icexelloss pushed a commit that referenced this issue Jun 21, 2023
@rtpsw
GH-35935: [C++] Clean interruption of a Acero plan with `use_threads=…
b451b34 
…false` (#35953)

### What changes are included in this PR?

The execution plan set-up code is refactored to call `StopProducing` on the plan in the reader's `Close`. Originally, `StopProducing` was called from the destructor of `BatchConverter`, but with `use_threads=false` a hang occurs prior to this point in the code.

### Are these changes tested?

Yes - a test that early-closes a plan reader.

### Are there any user-facing changes?

Yes; this will fix a hang visible to users.

* Closes: #35935

Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com>
Co-authored-by: rtpsw <rtpsw@hotmail.com>
Signed-off-by: Li Jin <ice.xelloss@gmail.com>
@icexelloss icexelloss added this to the 13.0.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rtpsw rtpsw

Labels
Component: C++ Type: enhancement
Projects
None yet
Milestone
13.0.0
Development

Successfully merging a pull request may close this issue.

GH-35935: [C++] Clean interruption of a Acero plan with use_threads=false rtpsw/arrow
2 participants
@icexelloss @rtpsw