Skip to content

[SPARK-26389][SS] Add force delete temp checkpoint configuration#23732

Closed
gaborgsomogyi wants to merge 3 commits into
apache:masterfrom
gaborgsomogyi:SPARK-26389
Closed

[SPARK-26389][SS] Add force delete temp checkpoint configuration#23732
gaborgsomogyi wants to merge 3 commits into
apache:masterfrom
gaborgsomogyi:SPARK-26389

Conversation

@gaborgsomogyi
Copy link
Copy Markdown
Contributor

@gaborgsomogyi gaborgsomogyi commented Feb 4, 2019

What changes were proposed in this pull request?

Not all users wants to keep temporary checkpoint directories. Additionally hard to restore from it.

In this PR I've added a force delete flag which is default false. Additionally not clear for users when temporary checkpoint directory deleted so added log messages to explain this a bit more.

How was this patch tested?

Existing + additional unit tests.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 4, 2019

Test build #102036 has finished for PR 23732 at commit 7c304f6.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gaborgsomogyi
Copy link
Copy Markdown
Contributor Author

gaborgsomogyi commented Feb 4, 2019

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 4, 2019

Test build #102039 has finished for PR 23732 at commit 7c304f6.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gaborgsomogyi
Copy link
Copy Markdown
Contributor Author

gaborgsomogyi commented Feb 4, 2019

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 5, 2019

Test build #102042 has finished for PR 23732 at commit 7c304f6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@camper42
Copy link
Copy Markdown

camper42 commented Feb 5, 2019 •
edited
Loading

thanks for quick improvement

@gaborgsomogyi
Copy link
Copy Markdown
Contributor Author

gaborgsomogyi commented Feb 5, 2019

cc @zsxwing @jose-torres

@jose-torres
Copy link
Copy Markdown
Contributor

jose-torres commented Feb 6, 2019

Two minor comments:

The flag description is a bit confusing; I would just describe the behavior when the flag is on, rather than saying it’s “normal” deletion.

I think the new flag should be read in DataStreamWriter, so the shouldDelete value we pass into StreamExecution will always be correct.

@gaborgsomogyi
Copy link
Copy Markdown
Contributor Author

gaborgsomogyi commented Feb 6, 2019 •
edited
Loading

The flag description is a bit confusing; I would just describe the behavior when the flag is on, rather than saying it’s “normal” deletion.

Clear, will do.

I think the new flag should be read in DataStreamWriter, so the shouldDelete value we pass into StreamExecution will always be correct.

shouldDelete flag is actually shouldDeleteIfQueryNotFailed and DataStreamWriter doesn't know whether the query stopped without exception. Do you mean change actual behaviour?

dongjoon-hyun
dongjoon-hyun reviewed Feb 7, 2019
View reviewed changes
Comment thread sql/core/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryManager.scala Outdated
@jose-torres
Copy link
Copy Markdown
Contributor

jose-torres commented Feb 7, 2019

You're right, there's no good way to move the flag the way I wanted. Looks good modulo Dongjoon's comments.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 7, 2019

Test build #102078 has finished for PR 23732 at commit ca77ebc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 8, 2019

Test build #102083 has finished for PR 23732 at commit ce2577d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

HeartSaVioR
HeartSaVioR reviewed Feb 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left minor comment on documentation.

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

val FORCE_DELETE_TEMP_CHECKPOINT_LOCATION =
buildConf("spark.sql.streaming.forceDeleteTempCheckpointLocation")
.doc("When true, enable temporary checkpoint locations force delete.")
Copy link
Copy Markdown
Contributor

@HeartSaVioR HeartSaVioR Feb 8, 2019

Choose a reason for hiding this comment

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

nit: I'd like to have this to be a valid and more described sentence like other configurations, especially this feature is only documented here. One example for me, When true, Spark always deletes temporary checkpoint locations for success queries.

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun Feb 8, 2019

Choose a reason for hiding this comment

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

Ur, @HeartSaVioR . That is the behavior before this PR.
The benefit of this configuration is to delete the locations when exception occurs.

Copy link
Copy Markdown
Contributor

@HeartSaVioR HeartSaVioR Feb 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes my bad I meant failed queries, maybe better, regardless of query's result - success or failure. Anyway this option is only documented here so I'd just wanted to make sure there's clear description.

dongjoon-hyun
dongjoon-hyun approved these changes Feb 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. This configuration is useful.
Merged to master.

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Feb 8, 2019

Thank you, @gaborgsomogyi , @camper42 , @jose-torres , @HeartSaVioR !

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@gaborgsomogyi
[SPARK-26389][SS] Add force delete temp checkpoint configuration
049fbfd 
## What changes were proposed in this pull request?

Not all users wants to keep temporary checkpoint directories. Additionally hard to restore from it.

In this PR I've added a force delete flag which is default `false`. Additionally not clear for users when temporary checkpoint directory deleted so added log messages to explain this a bit more.

## How was this patch tested?

Existing + additional unit tests.

Closes apache#23732 from gaborgsomogyi/SPARK-26389.

Authored-by: Gabor Somogyi <gabor.g.somogyi@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
mccheah pushed a commit to palantir/spark that referenced this pull request May 15, 2019
@gaborgsomogyi @mccheah
[SPARK-26389][SS] Add force delete temp checkpoint configuration
49dd067 
## What changes were proposed in this pull request?

Not all users wants to keep temporary checkpoint directories. Additionally hard to restore from it.

In this PR I've added a force delete flag which is default `false`. Additionally not clear for users when temporary checkpoint directory deleted so added log messages to explain this a bit more.

## How was this patch tested?

Existing + additional unit tests.

Closes apache#23732 from gaborgsomogyi/SPARK-26389.

Authored-by: Gabor Somogyi <gabor.g.somogyi@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
gatorsmile
gatorsmile reviewed Dec 23, 2019
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
.createOptional

val FORCE_DELETE_TEMP_CHECKPOINT_LOCATION =
buildConf("spark.sql.streaming.forceDeleteTempCheckpointLocation")
Copy link
Copy Markdown
Member

@gatorsmile gatorsmile Dec 23, 2019

Choose a reason for hiding this comment

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

Let us follow the other boolean conf naming convention?

spark.sql.streaming.forceDeleteTempCheckpointLocation.enabled?

Copy link
Copy Markdown
Member

@gatorsmile gatorsmile Dec 23, 2019

Choose a reason for hiding this comment

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

cc @Ngone51 could you submit a PR to make a change?

Copy link
Copy Markdown
Member

@Ngone51 Ngone51 Dec 23, 2019

Choose a reason for hiding this comment

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

Submitted #26981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HeartSaVioR HeartSaVioR HeartSaVioR left review comments

@gatorsmile gatorsmile gatorsmile left review comments

@Ngone51 Ngone51 Ngone51 left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@gaborgsomogyi @SparkQA @camper42 @jose-torres @dongjoon-hyun @HeartSaVioR @gatorsmile @Ngone51