Enable periodic cleanup of work_dir directories in ballista executor by Ted-Jiang · Pull Request #1783 · apache/datafusion

Ted-Jiang · 2022-02-08T11:25:57Z

Which issue does this PR close?

Closes#1780 .

Rationale for this change

Avoid work_dir quickly fill up disk space.

What changes are included in this PR?

like spark standalone mode, Executor periodic spawn a task to clean work_dir, if all the files in job_dir not modified in executor_cleanup_ttl seconds, it will be deleted.
https://spark.apache.org/docs/latest/spark-standalone.htmlspark.worker.cleanup.enabled

alamb · 2022-02-08T12:13:11Z

 .context("Could not connect to scheduler")?;

 let scheduler_policy = opt.task_scheduling_policy;
+ let cleanup_ttl = opt.executor_cleanup_ttl;


I wonder if it might be possible to use NamedTempFile or some other struct from tempfile (already a dependency): https://crates.io/crates/tempfile
There are at least two benefits:
The files are dropped immediately after they are no longer used so required intermediate diskspace is minimized
They can't be accidentally dropped while still in use (which perhaps affects long running queries)
I did something similar in DataFusion here: #1680

@alamb Thanks for your advice 😊!
IMHP, if one job has 3 stage, stage2 read stage1 input then delete the file, but stage2 task fail,
In ballista, scheduler will start a task to reload stage1 input. I think using NamedTempFile will cause some trouble and complexity.
we need keep the file for task-recovery and stage retry (like spark). So i decide if all the files under job_dir not modified in TTL we can safely delete it.
If i am not right, Please correct me 🙈

I don't honestly know enough about Ballista and its executor to know.
What do you think @yahoNanJing and @liukun4515 ?

realno · 2022-02-09T05:38:07Z

@Ted-Jiang this is a great step toward making Ballista production ready 👍 Thanks for making the change.

Ted-Jiang · 2022-02-09T06:40:26Z

cc @houqp @yahoNanJing

Ted-Jiang · 2022-02-16T07:17:44Z

cc @liukun4515

liukun4515 · 2022-03-04T13:14:38Z

+name = "executor_cleanup_enable"
+type = "bool"
+doc = "Enable periodic cleanup of work_dir directories."
+default = "true"


Can we set the default value to false and open it after a long time of testing?
@Ted-Jiang
If the default is false, this feature will not impact the current status. Users who want to use this feature can open it.
Do you agree? @alamb

I think it would be a good idea to put this feature in turned off by default so it can be tested more thoroughly

liukun4515 · 2022-03-04T13:31:09Z

+
+[[param]]
+name = "executor_cleanup_ttl"
+type = "i64"


it's better to use u64

Thanks fix !

alamb · 2022-03-04T18:56:10Z

Once someone who focuses on Ballista is happy with this PR I am happy to merge it

liukun4515 · 2022-03-06T12:29:42Z

Once someone who focuses on Ballista is happy with this PR I am happy to merge it

I think @Ted-Jiang will address the comments and please hold the status.
If I think it is ok, I will give my LGTM.

fix info Co-authored-by: Kun Liu <liukun@apache.org>

fix Co-authored-by: Kun Liu <liukun@apache.org>

liukun4515

Thanks @Ted-Jiang
LGTM

Ted-Jiang added 2 commits February 8, 2022 13:50

Enable periodic cleanup of work_dir directories in ballista executor
89d981d

add ut.
5742176

github-actionsBot added the ballista label Feb 8, 2022

alamb reviewed Feb 8, 2022
View reviewed changes

realno reviewed Feb 9, 2022
View reviewed changes

Ted-Jiang added 3 commits February 9, 2022 14:24

fix error handle.
f3718a4

fix clippy
4ac5f47

fix doc
e94f98a

realno reviewed Feb 9, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs Outdated

realno reviewed Feb 9, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs

Ted-Jiang added 2 commits February 15, 2022 11:46

add annotation
7b151b9

fix clippy
464efe0

liukun4515 reviewed Mar 4, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs Outdated

liukun4515 reviewed Mar 4, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs

liukun4515 reviewed Mar 4, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs Outdated

liukun4515 reviewed Mar 4, 2022
View reviewed changes

Comment threadballista/rust/executor/src/main.rs Outdated

Ted-Jiangand others added 4 commits March 7, 2022 15:19

Update ballista/rust/executor/src/main.rs
7aac84e
fix info Co-authored-by: Kun Liu <liukun@apache.org>

Update ballista/rust/executor/src/main.rs
b74770e
fix info Co-authored-by: Kun Liu <liukun@apache.org>

Update ballista/rust/executor/src/main.rs
ab6c41a
fix Co-authored-by: Kun Liu <liukun@apache.org>

change config
311f8ce

liukun4515 approved these changes Mar 8, 2022
View reviewed changes

alamb merged commit 432861e into apache:masterMar 8, 2022

yahoNanJing mentioned this pull request Oct 31, 2022
Long running stability apache/datafusion-ballista#466
Closed
9 tasks

Uh oh!

Conversation

Ted-Jiang commented Feb 8, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Uh oh!

alambFeb 8, 2022

Choose a reason for hiding this comment

Uh oh!

Ted-JiangFeb 9, 2022

Choose a reason for hiding this comment

Uh oh!

alambFeb 15, 2022

Choose a reason for hiding this comment

Uh oh!

realno commented Feb 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ted-Jiang commented Feb 9, 2022

Uh oh!

Uh oh!

Uh oh!

Ted-Jiang commented Feb 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liukun4515Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

alambMar 4, 2022

Choose a reason for hiding this comment

Uh oh!

liukun4515Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

Ted-JiangMar 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Mar 4, 2022

Uh oh!

liukun4515 commented Mar 6, 2022

Uh oh!

liukun4515 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants