[SPARK-26949][SS] Prevent 'purge' to remove needed batch files in CompactibleFileStreamLog #23850

HeartSaVioR · 2019-02-21T04:43:23Z

What changes were proposed in this pull request?

This patch proposes making purge in CompactibleFileStreamLog to throw UnsupportedOperationException to prevent purging necessary batch files, as well as adding javadoc to document its behavior. Actually it would only break when latest compaction batch is requested to be purged, but caller wouldn't be aware of this so safer to just prevent it.

How was this patch tested?

Added UT.

…pactibleFileStreamLog

HeartSaVioR · 2019-02-21T04:56:29Z

While I think it is safest way to only let CompactibleFileStreamLog to maintain logs, we have alternative options here:

Only purge batches which are safe to delete and don't throw exception

Suppose it has batch 0, 1, 2, 3, 4 which batch 2 is compacted. If 4 is given as parameter, purge will try to remove 0, 1, 2, 3 which removing batch 2 (latest compaction batch) would break the internal state. Instead of this, this method could be overridden to remove only batch 0 and 1 and silently ignore removing 2 and 3.
(I'm a bit hesitant on this cause sometimes it might be against caller's intention.)

Throw exception (maybe IllegalArgumentException, or IllegalStateException?) only when batches to purge contain the latest compaction batch

This would selectively throw exception - when it can break internal state of CompactibleFileStreamLog.

Please let me know if alternative would make more sense. Thanks in advance!

SparkQA · 2019-02-21T08:05:01Z

Test build #102575 has finished for PR 23850 at commit 9c5fa64.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2019-02-21T17:11:51Z

Seems like this PR is facing the same jenkins issues like mine and doesn't start new build.
BTW, just started to review this...

gaborgsomogyi · 2019-02-21T17:42:02Z

As long as the functionality is not needed I'm fine with the UnsupportedOperationException.
I've double checked and not used anywhere.

As a potential user of this or any kind of function I expect to do what I want without any internal thinking. Because of this option 1 and 2 are having side effects from my perspective.

If it would be really required I think the compacted file can be deserialized => remove the batches => serialized again with proper content (maybe you meant the same by maintain logs).

HeartSaVioR · 2019-02-21T21:27:53Z

Thanks @gaborgsomogyi for providing your opinion. Same here, and that's why I took this approach and left such approaches as alternatives.

If it would be really required I think the compacted file can be deserialized => remove the batches => serialized again with proper content (maybe you meant the same by maintain logs).

What I meant by maintain logs is compaction what CompactibleFileStreamLog is doing now. IMHO instrumenting latest compacted batch doesn't seem to be an alternative, because we can't find which entries were added before thresholdBatchId once batches were compacted.

HeartSaVioR · 2019-02-21T21:36:42Z

Btw, purgeAfter would be safe since it'll leave compaction batch which batch id is equals or less than thresholdBatchId, or otherwise remaining batches don't need compaction batch (batches before first compaction).

gaborgsomogyi · 2019-02-21T22:19:59Z

we can't find which entries were added before thresholdBatchId once batches were compacted

Yeah, that's true. Batch ID is not compacted into the file.

SparkQA · 2019-02-21T22:41:45Z

Test build #102587 has finished for PR 23850 at commit a51564f.

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

gaborgsomogyi

LGTM.

HeartSaVioR · 2019-02-25T05:19:48Z

cc. @zsxwing @brkyvz @jerryshao since they've authored parts of the file.

HeartSaVioR · 2019-03-14T12:59:55Z

Ping.

HeartSaVioR · 2019-03-17T21:46:43Z

Also cc-ing @tdas and @jose-torres since CompactibleFileStreamLog is only used for SS.

HeartSaVioR · 2019-03-25T01:29:45Z

Kindly reminder.

HeartSaVioR · 2019-04-06T07:45:02Z

Ping again.

HeartSaVioR · 2019-04-30T20:41:44Z

Ping again, as Spark+AI Summit 2019 in SF is end.

felixcheung · 2019-06-02T18:36:57Z

where are we on this? @tdas and @jose-torres

SparkQA · 2019-06-02T21:05:08Z

Test build #106067 has finished for PR 23850 at commit a51564f.

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

HeartSaVioR · 2019-06-03T06:16:39Z

When this is revisited, please consider other PRs in mine as well: https://github.com/apache/spark/pulls/HeartSaVioR
And also consider other PRs in SS area, too: https://spark-prs.appspot.com/open-prs#streaming (title starts with [SS])

dongjoon-hyun · 2019-06-09T03:43:14Z

Retest this please.

dongjoon-hyun · 2019-06-09T03:50:34Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/CompactibleFileStreamLog.scala

+   * of given parameter, and let CompactibleFileStreamLog handles purging by itself.
+   */
+  override def purge(thresholdBatchId: Long): Unit = throw new UnsupportedOperationException(
+    s"'purge' might break internal state of CompactibleFileStreamLog hence not supported")


nit. s" -> "?

CompactibleFileStreamLog hence not supported seems to need some revision.

dongjoon-hyun

Hi, @HeartSaVioR .
This looks like a mismatch between compactInterval and the parameter of purge.
I have a question. If CompactibleFileStreamLog calls purge only when isCompactionBatch returns true, does purge fail in that case?

SparkQA · 2019-06-09T06:45:29Z

Test build #106320 has finished for PR 23850 at commit a51564f.

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

HeartSaVioR · 2019-06-12T08:37:44Z

@dongjoon-hyun
Thanks for taking a look at the patch.

If CompactibleFileStreamLog calls purge only when isCompactionBatch returns true, does purge fail in that case?

Let me clear the issue - the condition which breaks internal state is, batches to purge contain the latest compaction batch, as further batches will refer the compaction batch.

I've described alternatives as well, so please take a look at previous comment: #23850 (comment)

Btw, even we could purge batches earlier than latest compaction batch, CompactibleFileStreamLog also does the clean up in deleteExpiredLog so it is actually not needed. (I'd like to let CompactibleFileStreamLog be responsible to take care about logs by itself.)

SparkQA · 2019-06-12T10:53:19Z

Test build #106412 has finished for PR 23850 at commit ba62794.

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

SparkQA · 2019-06-12T15:36:35Z

Test build #106422 has finished for PR 23850 at commit f8adab3.

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

dongjoon-hyun

This PR makes the abstract class CompactibleFileStreamLog more robust by preventing error situations. The derived classes like FileStreamSinkLog should override this method with a safe and correct implementation.

@tdas , @jose-torres , @cloud-fan , @gatorsmile , @HyukjinKwon . This PR looks reasonable to me. I'm supporting @HeartSaVioR 's suggestion. Please give us your comments (for better alternatives or any regressions)

If there is no more comments, I'll proceed to merge this improvement to master for Apache Spark 3.0.0 in a few days.

dongjoon-hyun · 2019-06-15T03:33:51Z

Merged to master. Thank you, @HeartSaVioR and @gaborgsomogyi .
cc @tdas , @jose-torres , @zsxwing

…pactibleFileStreamLog ## What changes were proposed in this pull request? This patch proposes making `purge` in `CompactibleFileStreamLog` to throw `UnsupportedOperationException` to prevent purging necessary batch files, as well as adding javadoc to document its behavior. Actually it would only break when latest compaction batch is requested to be purged, but caller wouldn't be aware of this so safer to just prevent it. ## How was this patch tested? Added UT. Closes apache#23850 from HeartSaVioR/SPARK-26949. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

HeartSaVioR · 2019-06-15T14:04:13Z

Thanks all for reviewing and merging!

[SPARK-26949][SS] Prevent 'purge' to remove needed batch files in Com…

9c5fa64

…pactibleFileStreamLog

Fix a silly mistake

a51564f

gaborgsomogyi approved these changes Feb 22, 2019

View reviewed changes

HeartSaVioR mentioned this pull request Feb 25, 2019

[WIP][DISCUSSION_NEEDED][SPARK-24295][SS] Add option to retain only last batch in file stream sink metadata #23840

Closed

dongjoon-hyun reviewed Jun 9, 2019

View reviewed changes

Refine sentence on exception

ba62794

Fix UT

f8adab3

dongjoon-hyun approved these changes Jun 12, 2019

View reviewed changes

dongjoon-hyun added the STRUCTURED STREAMING label Jun 14, 2019

dongjoon-hyun closed this in bd0a04b Jun 15, 2019

HeartSaVioR deleted the SPARK-26949 branch June 15, 2019 14:04

[SPARK-26949][SS] Prevent 'purge' to remove needed batch files in CompactibleFileStreamLog #23850

[SPARK-26949][SS] Prevent 'purge' to remove needed batch files in CompactibleFileStreamLog #23850

Uh oh!

Conversation

HeartSaVioR commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HeartSaVioR commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 21, 2019

Uh oh!

gaborgsomogyi commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborgsomogyi commented Feb 21, 2019

Uh oh!

HeartSaVioR commented Feb 21, 2019

Uh oh!

HeartSaVioR commented Feb 21, 2019

Uh oh!

gaborgsomogyi commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 21, 2019

Uh oh!

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Feb 25, 2019

Uh oh!

HeartSaVioR commented Mar 14, 2019

Uh oh!

HeartSaVioR commented Mar 17, 2019

Uh oh!

HeartSaVioR commented Mar 25, 2019

Uh oh!

HeartSaVioR commented Apr 6, 2019

Uh oh!

HeartSaVioR commented Apr 30, 2019

Uh oh!

felixcheung commented Jun 2, 2019

Uh oh!

SparkQA commented Jun 2, 2019

Uh oh!

HeartSaVioR commented Jun 3, 2019

Uh oh!

dongjoon-hyun commented Jun 9, 2019

Uh oh!

dongjoon-hyun Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 9, 2019

Uh oh!

HeartSaVioR commented Jun 12, 2019

Uh oh!

SparkQA commented Jun 12, 2019

Uh oh!

SparkQA commented Jun 12, 2019

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 15, 2019

Uh oh!

HeartSaVioR commented Jun 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

HeartSaVioR commented Feb 21, 2019 •

edited

Loading

HeartSaVioR commented Feb 21, 2019 •

edited

Loading

gaborgsomogyi commented Feb 21, 2019 •

edited

Loading

gaborgsomogyi commented Feb 21, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading