Skip to content

[SPARK-58883][CORE] Detect truncated checkpoint directory on read via persisted partition count - #58167

Open
anshulbaliga7 wants to merge 3 commits into
apache:masterfrom
anshulbaliga7:SPARK-58883-checkpoint-partition-count
Open

anshulbaliga7 wants to merge 3 commits into
apache:masterfrom
anshulbaliga7:SPARK-58883-checkpoint-partition-count

Conversation

@anshulbaliga7

@anshulbaliga7 anshulbaliga7 commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Persist the original RDD's partition count to a _num_partitions file in the checkpoint directory at write time, and validate it in ReliableCheckpointRDD.getPartitions on read. A missing file (pre-existing checkpoints) or a file that fails validation is tolerated for backward compatibility, with a WARN logged in the latter case.

The file is 13 bytes: a 1-byte format version, a 4-byte big-endian partition count, and an 8-byte CRC32 over the preceding 5 bytes. It is written to a temp path and renamed into place so a torn write leaves no partial file. The reader rejects an unknown version, checksum mismatch, negative count, or trailing bytes, so corrupted metadata degrades to "detection inactive" rather than a false truncation error. Publication is best effort: a create or rename failure is logged with its cause and does not fail the action that triggered checkpointing.

The check can be disabled with spark.checkpoint.verifyPartitionCount.enabled=false (internal, default true) to load a legitimately truncated directory as-is.

Why are the changes needed?

getPartitions only checks that part-* files are contiguous from part-00000. Deleting a middle file breaks contiguity and is caught, but deleting the trailing file leaves the rest contiguous, so the check passes and the RDD is silently read back with fewer partitions and no error. This mainly affects SparkContext.checkpointFile, which Spark Streaming recovery uses to rebuild generatedRDDs, since that path has no original RDD to compare against.

Split out of SPARK-58770 (#58004), which deliberately left this out as needing its own on-disk format.

Does this PR introduce any user-facing change?

Yes. Checkpoint directories now get one extra small metadata file. Reading back a directory whose part-file count disagrees with the recorded count now throws CHECKPOINT_DIRECTORY_PARTITION_COUNT_MISMATCH, with sub-condition MISSING_FILES when trailing files are lost and UNEXPECTED_FILES when extra contiguous files are present. Both name the directory, the expected count, and the count found. Directories without the file (written by older Spark, or where the best-effort write failed) read exactly as before. Older Spark versions ignore the new file since getPartitions already filters on the part- prefix.

How was this patch tested?

Added tests to CheckpointStorageSuite:

  • trailing partition file deleted throws MISSING_FILES, and loads as-is with the config disabled
  • extra contiguous partition file added throws UNEXPECTED_FILES, and loads all files with the config disabled
  • a checkpoint missing _num_partitions still reads back correctly
  • a corrupted _num_partitions file is tolerated with a WARN, covering a short garbage payload, count bytes rewritten under an intact checksum, trailing bytes, and checksum-valid payloads with an unsupported version or a negative count; the exact 13-byte version-1 encoding is pinned against a hardcoded literal, and a control confirms real truncation is still reported
  • checkpointing succeeds, logs the inactive-check WARN, and leaves neither the final nor the temp file behind when publication fails, parameterized over: create throws, write throws after create, rename returns false, rename throws
  • a false result from temp-file delete after a publication failure logs a cleanup WARN, and the directory still recovers

Was this patch authored or co-authored using generative AI tooling?

Yes.

@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

@LuciferYang @dongjoon-hyun Please have a look. Thanks!! (Note: currently throws _LEGACY_ERROR_TEMP_3019 since #58004 hasn't merged yet...will rebase to CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH once it does.)

@anshulbaliga7
anshulbaliga7 force-pushed the SPARK-58883-checkpoint-partition-count branch 2 times, most recently from c3f64d0 to f27bdf2 Compare August 21, 2026 13:28
@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

Gentle ping @LuciferYang @dongjoon-hyun !

@LuciferYang LuciferYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The gap this closes is real: delete the trailing part-* and the rest stay contiguous, so the old check passes and the RDD comes back a partition short. I left six comments inline; none of them block.

The one I would settle before the on-disk format ships is the first. Writing _num_partitions and reading it both tolerate any failure, but acting on it is fatal and has no switch. Two of the other comments, on the swallowed write error and on the non-atomic write, cover the remaining ways the check can end up disabled without anyone noticing.

Compatibility in the other direction looks fine: the startsWith("part-") filter in getPartitions is older than _partitioner, so an older Spark reading a directory written by this version ignores the new file. The format has no version field, though, so a later change to the payload is only safe if it also changes the filename. An older reader that still deserializes the bytes to an Int will go on comparing that Int as a partition count.

// silently tolerated for backward compatibility. See SPARK-58883.
ReliableCheckpointRDD.readPartitionCountFromCheckpointDir(context, checkpointPath)
.foreach { expected =>
if (inputFiles.length != expected) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Writing and reading _num_partitions both tolerate any failure (:312, :346), but acting on it (:93) is fatal and consults no config, and the diff does not touch internal/config. The analogous local-checkpoint integrity feature in this same release ships behind spark.checkpoint.local.verifyChecksum.enabled, an .internal() flag defaulting to true.

There is no API-level way around the throw either; neither version of checkpointFile is public. What is left is deleting _num_partitions from storage, and since the error names no directory, that means deleting it from every rdd-*, which turns the check off wholesale. Until someone does that, a streaming app fails on every restart. Gating this comparison the same way would cover it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No bypass for a fatal check. Added spark.checkpoint.verifyPartitionCount.enabled, .internal(), defaults to true, following the spark.checkpoint.local.verifyChecksum.enabled precedent you pointed to.

Setting it to false loads a truncated directory as-is instead of throwing..

logDebug(s"Written partition count $partitionCount to $countFilePath")
} catch {
case NonFatal(e) =>
logWarning(log"Error writing partition count to ${MDC(PATH, checkpointDirPath)}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The catch in writePartitionCountToCheckpointDir never uses e (:312), so a failed write leaves one WARN with no cause. The read side treats a missing _num_partitions as a directory written by an older Spark (:343), so once the file fails to appear, truncation detection stays off for that directory while rdd.checkpoint() still returns success.

The minimum is to pass e to logWarning, add the part-file count found in the directory, and say in the scaladoc that a failed write leaves the check inactive. Letting the write throw is the stronger option, but it is not free: doCheckpoint() runs after dagScheduler.runJob has returned, so throwing fails an action whose result is already computed, and cpState stays at CheckpointingInProgress.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Swallowed e in the write-failure WARN. Now passed through, and the message states plainly that truncation detection is inactive for that directory going forward.

case _: FileNotFoundException =>
logDebug(s"No partition count file in $checkpointDirPath (older checkpoint)")
None
case NonFatal(e) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_num_partitions is written with no atomic replace: part files go to .part-NNNNN-attempt-K and are renamed (:217, :246), while this one, like _partitioner, is a plain fs.create (:302, :272). The difference is the consequence: losing the partitioner costs a shuffle, losing this file turns off the check the PR just added. If the driver dies mid-write, or the storage truncates it, the read at :346 swallows the NonFatal into None and one WARN is all that is left. That is the same storage this PR assumes can drop a trailing file.

Routing the Int through DataOutputStream.writeInt plus a format version, written to a temp path and renamed, makes it a fixed 8 bytes whose length shows a torn write.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Non-atomic write. Rewrote to DataOutputStream.writeInt behind a 1-byte format version, written to a temp path and renamed into place. A rename failure is also logged (with cause) rather than silently dropped, so every path that disables the check now leaves a trace.

.foreach { expected =>
if (inputFiles.length != expected) {
throw SparkCoreErrors.checkpointRDDHasDifferentNumberOfPartitionsFromOriginalRDDError(
id, expected, id, inputFiles.length)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:95 passes the same id into both RDD-id placeholders of the template, so the message reads The checkpoint of RDD 4 ... The checkpoint RDD is 4 and the original RDD's id is gone, while :199 passes originalRDD.id and newRDD.id. The second message line still carries the diagnosis hint, so it is not misleading, but neither variant names the checkpoint directory.

The write path goes through the new check too: :93 and :197 test the same condition, so the new one throws first and the old one is only reachable when _num_partitions could not be written or read back. This mismatch should get its own error condition carrying the directory path and the two counts; the neighboring INVALID_CHECKPOINT_DIRECTORY (:85) already carries path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same RDD id in both template slots, no directory in the message. Split into a new condition, CHECKPOINT_TRUNCATED_DIRECTORY, scoped to this read-only check that carries path, expected, found. CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH is untouched and still only reachable from the write-path comparison.


// Remove the metadata file to simulate a pre-SPARK-58883 checkpoint.
val countFile = new Path(checkpointPath, "_num_partitions")
fs.delete(countFile, false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Delete the write at ReliableCheckpointRDD.scala:189 and only the first of the three tests goes red. Test 2 deletes _num_partitions (:795) and test 3 overwrites it (:818), neither asserting the file exists first, and fs.delete returning false is ignored while fs.create(path, true) on a missing path is silent. So those two show that reading works, not the backward compatibility and corruption tolerance their comments claim.

Test 1 does guard its precondition (:760), and so does an older test in the same file (:707). One assert(fs.exists(countFile)) in each of tests 2 and 3 covers it, and asserting the WARN in test 3 with withLogAppender is what separates "corruption tolerated" from "the check never ran".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tests 2 and 3 didn't prove what their comments claimed so added assert(fs.exists(...)) preconditions to both, and test 3 now asserts the WARN fires via withLogAppender, so it actually distinguishes "corruption tolerated" from "the check never ran."

val fs = countFilePath.getFileSystem(sc.hadoopConfiguration)
// overwrite = false: matches _partitioner's write helper; a second checkpoint to the
// same directory would fail here (caught and logged below), which is acceptable.
val fileOutputStream = fs.create(countFilePath, false, bufferSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The two comment lines above :302 justify overwrite = false with "a second checkpoint to the same directory would fail here", but that case is not reachable. The directory is <dir>/<UUID>/rdd-<rddId>, the UUID changes on every setCheckpointDir, rddId is unique within a SparkContext, writeRDDToCheckpointDirectory has exactly one call site (ReliableRDDCheckpointData.scala:61), and RDDCheckpointData.checkpoint() gates on cpState so it runs once.

If it were reachable the conclusion would invert too: overwrite = false keeps the first value, so every later read compares a stale count against a new directory and throws a mismatch as soon as the two disagree. "Keeps this consistent with _partitioner" is all the comment needs to say.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The overwrite = false reasoning is moot now since its replaced by the temp-file-then-rename write.

@anshulbaliga7
anshulbaliga7 force-pushed the SPARK-58883-checkpoint-partition-count branch from f27bdf2 to 40a90e8 Compare August 26, 2026 10:39
@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @LuciferYang , have addressed all six comments above.

Can you PTAL again? Thanks!

@anshulbaliga7
anshulbaliga7 force-pushed the SPARK-58883-checkpoint-partition-count branch from 40a90e8 to ea2cbeb Compare August 26, 2026 13:51
@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

@LuciferYang gentle ping, can you please take a look? Thanks!

@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

@LuciferYang just following up on the above. All six comments are addressed and rebased.

CI is green. Would appreciate another look when you have time. Thank you!

@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

@LuciferYang @cloud-fan Gentle Ping!

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary

The current patch addresses all six comments from my previous review: it adds the recovery switch, preserves write failures in warnings, uses versioned temp-and-rename publication, introduces a dedicated path-and-count error, strengthens the metadata tests, and removes the obsolete overwrite rationale. I found two additional non-blocking issues in the current format and test boundary: valid-layout count corruption can still cause a false fatal mismatch, and the best-effort metadata publication paths have no failure-injection coverage.

Findings

2 total: 0 P0, 0 P1, 2 P2, 0 P3.

Non-blocking (P2)

  • Validate valid-layout metadata corruption — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:351 — see inline.
  • Cover best-effort publication failures — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:317 — see inline.

PR description suggestions

  • Update the user-facing change section to name CHECKPOINT_TRUNCATED_DIRECTORY instead of the superseded CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH error.

throw new IllegalArgumentException(
s"Unsupported _num_partitions format version: $version in $countFilePath")
}
dis.readInt()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): readInt() makes every version-1 five-byte payload authoritative. If the count bytes of an intact four-part checkpoint are corrupted to another valid Int, such as 5, this path throws CHECKPOINT_TRUNCATED_DIRECTORY instead of taking the corrupt-metadata fallback; the added corruption test covers only a short payload that fails parsing. Please make the unreleased payload self-validating and reject corrupted count bytes before comparing them, while preserving the real trailing-file mismatch behavior.

} {
dos.close()
}
if (!fs.rename(tmpFilePath, countFilePath)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): The best-effort publication contract is not regression-tested here: all new cases use a filesystem where _num_partitions creation and rename succeed. Please add failure-injection coverage for both a metadata create/write exception and a false rename result, asserting that checkpointing still completes and the inactive-check warning is emitted. Otherwise either branch could later start failing an action after its partition files were already committed without making this suite red.

@cloud-fan

Copy link
Copy Markdown
Contributor

Thanks for the ping. I am responding with a review of the current head.

@anshulbaliga7
anshulbaliga7 force-pushed the SPARK-58883-checkpoint-partition-count branch from ea2cbeb to ef2855d Compare September 22, 2026 19:22
@anshulbaliga7
anshulbaliga7 force-pushed the SPARK-58883-checkpoint-partition-count branch from ef2855d to 597f2ae Compare September 22, 2026 19:32
@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

Thanks @cloud-fan for the review, have addressed both as:

  • The payload now carries a CRC32 over the version and count bytes (13 bytes total), and the reader rejects a version mismatch, checksum mismatch, negative count, or trailing bytes before comparing. Any rejection takes the tolerated "detection inactive" path. The corruption test now includes your exact case: the count bytes rewritten to 5 with the recorded checksum left intact, which is tolerated, plus a control that restores the original payload and confirms a real missing trailing file still throws CHECKPOINT_TRUNCATED_DIRECTORY.

  • Added two failure-injection tests using LocalFileSystem subclasses, following the existing MkdirsFailingFilesystem pattern: one throws from create on the _num_partitions temp file, one returns false from rename onto the final path. Both assert the action completes, the RDD is checkpointed, the inactive-check WARN is logged, neither the final nor the temp file is left behind, and the directory reads back as a pre-SPARK-58883 checkpoint.

PR description updated to name CHECKPOINT_TRUNCATED_DIRECTORY, the on-disk format, and the spark.checkpoint.verifyPartitionCount.enabled switch.

Can you PTAL again? Thanks!

@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

Gentle ping @cloud-fan @LuciferYang !

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary

The two findings from the prior review are addressed on this head: count-byte corruption is now detected by a checksum, and create plus rename-false publication failures have focused coverage. This round found two further non-blocking issues: exceptions after the metadata temp file is created can leave that file behind without any test reaching the path, and the new durable format tests do not independently pin the declared version-1 fields or checksum-valid invalid values.

Findings

2 total: 0 P0, 0 P1, 2 P2, 0 P3.

Non-blocking (P2)

  • Clean up the partition-count temp file after publication exceptions — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:365 — see inline.
  • Pin the partition-count format independently in tests — core/src/test/scala/org/apache/spark/CheckpointSuite.scala:862 — see inline.

Re-review status

Prior AI findings: 2 addressed, 0 still present; additional unresolved findings in this review: 2.

New attribution: 1 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.

Remaining prior AI findings

No prior AI findings remain.

Existing discussions

  • existing discussion — The author reports that the tested create-throw and rename-false branches leave no temp file, but the current helper still leaves it behind when write, close, or rename throws after creation.
  • existing discussion — The added cases prove a create exception and a false rename result remain non-fatal and warn, but they never inject write, close, or throwing-rename failures after the temp file exists. The current suite therefore misses the exceptional cleanup modes that expose the retained production defect.

} else {
logDebug(s"Written partition count $partitionCount to $countFilePath")
}
} catch {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): After fs.create succeeds, dos.write, dos.close, or fs.rename can throw into this catch. It logs and returns without deleting ._num_partitions-tmp, leaving an orphan artifact on a deliberately tolerated failure path and violating the stated no-partial-file contract; the current create-throw and rename-false cases do not exercise this path.

Verification:

  • Regression: Inject at least one metadata publication exception after the temp path exists and verify successful checkpoint completion, the inactive-check warning, absence of both metadata paths, and successful recovery.
  • Behavior: Verify existing successful publication and false-return rename behavior remain unchanged while cleanup failures do not mask the original publication failure.

assert(fs.exists(countFile), "expected _num_partitions to exist before corruption")
val originalBytes = readAllBytes(fs, countFile)
// Format version 1: 1-byte version, 4-byte count, 8-byte CRC32 of the first 5 bytes.
assert(originalBytes.length === 13)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): This assertion pins only the total length; every subsequent case starts from bytes emitted by the same writer and is interpreted by the paired reader. A coordinated change to the version byte, count endianness, or stored CRC can remain green without a version bump, and checksum-valid unsupported-version or negative-count payloads do not exercise their rejection branches.

Verification:

  • Compatibility: Assert the exact semantic fields of a version-1 partition-count payload using an independently computed expected encoding.
  • Regression: Construct checksum-valid unsupported-version and negative-count payloads and verify the inactive-check warning and successful recovery with the actual checkpoint partitions.

@cloud-fan

Copy link
Copy Markdown
Contributor

Thanks for the ping. I reviewed the current head and posted the result.

…the v1 format independently in tests, per review
@anshulbaliga7

anshulbaliga7 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @cloud-fan for the review, have addressed both in bc5ac83.

  • Temp-file cleanup: The write and rename now run inside an inner try; any exception after the temp file is created deletes it before rethrowing to the existing best-effort catch, which still logs the inactive-check warning. The delete is itself guarded so a cleanup failure is logged and never masks the original publication failure. The rename-false path reuses the same helper and successful publication is unchanged. Two new failure-injection cases cover this: an output stream that throws on the first write to the temp file, and a rename onto the final path that throws instead of returning false. All four cases assert the action completes, the WARN fires and neither _num_partitions nor its temp file remains and the directory recovers with the actual partitions.

  • Independent format pin: The corruption test now asserts the exact 13-byte version-1 payload for count 4 against a hardcoded literal (version, big-endian count, big-endian CRC32 of the first 5 bytes), so a coordinated writer and reader change without a version bump goes red. A test-side encoder, checked against that same literal, builds checksum-valid payloads with an unsupported version and with a negative count and both are asserted to hit the tolerated path with the WARN and full recovery.

Can you PTAL again? Thanks!

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary

Both findings from the previous round are addressed: post-create write and rename exceptions now attempt temporary-file cleanup, and the format tests independently pin the version-1 bytes plus checksum-valid invalid values. Five newly surfaced issues remain on this head. The setting documentation incorrectly implies that the switch controls metadata writes. The mismatch error is misleading when unexpected extra partition files are present, and that surplus-file path lacks regression coverage. Missing metadata is treated as proof of an older checkpoint even though current best-effort publication can fail. Finally, a false return from temporary-file deletion is silently ignored.

Review-level concerns

  • Shared problem: The best-effort metadata lifecycle is described independently by the config entry and reader diagnostics, and correcting only one leaves contradictory write-versus-read semantics. As a non-blocking boundary response, align the unconditional writer attempt, reader-only switch, and ambiguous missing-metadata fallback under one lifecycle contract.
  • Shared problem: The reader inequality, structured error path, and regression portfolio need one direction-aware count-mismatch contract; isolated changes leave either misleading surplus-file output or uncovered enforcement. As a non-blocking response, distinguish deficit from surplus and pin both reachable directions.

Findings

5 total: 0 P0, 0 P1, 5 P2, 0 P3.

Non-blocking (P2)

  • Describe the setting as a read-time verification switch — core/src/main/scala/org/apache/spark/internal/config/package.scala:3173 — see inline.
  • Make the mismatch diagnostic accurate for extra files — common/utils/src/main/resources/error/error-conditions.json:1081 — see inline.
  • Cover the unexpected-extra-partition mismatch path — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:98 — see inline.
  • Account for current writers that fail to publish metadata — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:74 — see inline.
  • Report a false result from temporary-file cleanup — core/src/main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala:351 — see inline.

Re-review status

Prior AI findings: 2 addressed, 0 still present; additional unresolved findings in this review: 5.

New attribution: 0 newly introduced, 5 late catch, 0 previously raised, 0 unattributed.

Remaining prior AI findings

No prior AI findings remain.

Existing discussions

PR description suggestions

  • Update the testing section to include the new write-failure and throwing-rename publication cases, and replace the stale statement that five tests were added with wording that matches the current parameterized coverage.

private[spark] val CHECKPOINT_VERIFY_PARTITION_COUNT_ENABLED =
ConfigBuilder("spark.checkpoint.verifyPartitionCount.enabled")
.internal()
.doc("When true, a checkpoint directory written by a Spark version that supports " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): The writer publishes _num_partitions regardless of this setting; only getPartitions checks the flag. An application can therefore write with the flag disabled and later have the same metadata enforced by a default-configured reader. Could this text state that publication is always attempted best-effort and that the flag only bypasses the read-time comparison?

Verification:

  • Inspection: The configuration text agrees with the unconditional writer call and the reader-only configuration branch for both enabled and disabled values.

},
"CHECKPOINT_TRUNCATED_DIRECTORY" : {
"message" : [
"The checkpoint directory <path> was written with <expected> partition(s) but only <found> partition file(s) were found. One or more trailing part files may have been lost.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): This message is also used when found > expected. For example, a valid count of 4 plus contiguous part-00004 produces found=5, yet the error says only five files were found and that files may have been lost. Please use direction-neutral mismatch wording or distinguish missing files from unexpected extra files.

Verification:

  • Behavior: Valid metadata with fewer contiguous part files and valid metadata with extra contiguous part files produce direction-appropriate structured failures with the correct path and counts.
  • Compatibility: Disabled verification and absent or invalid metadata continue to bypass the new comparison exactly as before.

if (context.conf.get(CHECKPOINT_VERIFY_PARTITION_COUNT_ENABLED)) {
ReliableCheckpointRDD.readPartitionCountFromCheckpointDir(context, checkpointPath)
.foreach { expected =>
if (inputFiles.length != expected) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): The tests only cover this condition with fewer files than recorded. A regression from != to < would therefore leave all of them green. Please add a case with a recorded count of 4 and contiguous parts part-00000 through part-00004, and assert the greater-than mismatch details.

Verification:

  • Regression: A checkpoint with recorded count four and contiguous parts zero through four is rejected with the expected greater-than mismatch details.

* Since the original RDD may belong to a prior application, there is no way to know a
* priori the number of partitions to expect. This method assumes that the original set of
* checkpoint files are fully preserved in a reliable storage across application lifespans.
* When the directory was written by a Spark version that supports SPARK-58883, a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): A current writer can also omit this file because publication is best-effort and any publication failure is tolerated. The reader then reaches the same missing-file path, so describing absence as proof of an older checkpoint is inaccurate. Please cover both legacy checkpoints and failed best-effort publication in the documentation and diagnostic.

Verification:

  • Inspection: Every changed statement about missing metadata is consistent with the writer catch path that completes checkpointing without the final metadata file.
  • Compatibility: A missing metadata file remains non-authoritative and recovery continues without partition-count enforcement.

// logged.
def deleteTempFile(): Unit = {
try {
fs.delete(tmpFilePath, false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking (P2): FileSystem.delete can return false without throwing when the temp file was not removed. Ignoring that result means a post-create publication failure can leave ._num_partitions-tmp behind without the promised cleanup warning. Please check the Boolean result, warn when it is false, and cover that failure signal in the injected-filesystem tests.

Verification:

  • Regression: After a post-create publication failure, an injected false delete result leaves checkpointing recoverable and emits both publication-failure and cleanup-failure evidence.
  • Behavior: A successful cleanup removes the temporary metadata path without adding a cleanup warning.

@cloud-fan

Copy link
Copy Markdown
Contributor

Thanks, both issues from the previous round are addressed in bc5ac83: post-create publication exceptions clean the temp path, and the tests independently pin version 1 plus checksum-valid invalid payloads.

…e delete result, and fix the metadata lifecycle docs, per review
@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

Thanks @cloud-fan for the review , have addressed all five:

  • Setting doc: Rewritten as a read-time switch where publication is always attempted best-effort regardless of the value, the flag skips only the comparison and it has no effect when the file is absent for either reason.

  • Direction-aware mismatch: Replaced CHECKPOINT_TRUNCATED_DIRECTORY with CHECKPOINT_DIRECTORY_PARTITION_COUNT_MISMATCH and two sub-conditions, MISSING_FILES and UNEXPECTED_FILES, selected by comparing found against recorded. The parent message is direction-neutral with path and both counts; each sub-condition carries the direction-specific diagnosis and the bypass hint. I renamed rather than only rewording since "truncated" is wrong for the surplus case.

  • Surplus coverage: New test with recorded count 4 and contiguous part-00000 through part-00004 asserts UNEXPECTED_FILES with expected 4, found 5, and that the disabled switch loads all 5. The existing missing-file tests now assert MISSING_FILES.

  • Missing metadata: The getPartitions scaladoc and comment, the reader scaladoc and the debug log now name both causes an older Spark version or a failed best-effort write by a current one.

  • Delete result: A false return from delete now logs the cleanup warning, same as an exception. The four existing publication-failure cases additionally assert no cleanup warning is emitted. A new case injects a throwing rename plus a false delete and asserts both the publication and cleanup warnings, the leftover temp file, and successful recovery with 4 partitions.

PR description updated for the renamed condition and the current test coverage.

Can you PTAL again? Thanks!

@anshulbaliga7

Copy link
Copy Markdown
Contributor Author

@LuciferYang this has been through four review rounds since your Aug 26 pass, and every item from them is addressed, including the recovery switch, atomic versioned publication with a checksum, direction-aware error conditions, and failure-injection coverage. Would you be able to take one more look? If the design has grown past what you'd want for this feature, I'm happy to trim it back.

@LuciferYang LuciferYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The earlier comments are all addressed, and the format and the failure handling look right to me now. Two of the four inline comments I would settle before merging: the flag does not take effect on DStream recovery, and on the write path the new check masks the SPARK-58770 diagnosis. Both are small changes. The other two are not blocking; the one on temp-then-rename is my answer to your question about trimming back.

// or failed best-effort write) is tolerated. Set
// spark.checkpoint.verifyPartitionCount.enabled=false to skip only this comparison when
// the directory must be loaded as-is. See SPARK-58883.
if (context.conf.get(CHECKPOINT_VERIFY_PARTITION_COUNT_ENABLED)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new flag does not take effect on DStream recovery, which is the only production caller of checkpointFile. On getOrCreate, the SparkContext is built from Checkpoint.createSparkConf(), which starts from the conf saved in the checkpoint and re-reads only a fixed set of keys, propertiesToReload, from the current environment. I checked this in spark-shell on 4.1.1 (that code is unchanged since): restarting with --conf spark.checkpoint.verifyPartitionCount.enabled=false leaves the key unset in the recovered conf, while spark.master, which is on that list, picks up the new value. So an operator who follows the error message and restarts with the flag off gets the same error again.

Adding CHECKPOINT_VERIFY_PARTITION_COUNT_ENABLED.key to propertiesToReload in Checkpoint.scala would fix it; UI_FILTERS.key in that list already references a core config the same way. The streaming CheckpointSuite test "recovery of conf through checkpoints" already drives createSparkConf() through system properties, so one more case there would pin it.

if (originalRDD.partitioner.nonEmpty) {
writePartitionerToCheckpointDir(sc, originalRDD.partitioner.get, checkpointDirPath)
}
writePartitionCountToCheckpointDir(sc, originalRDD.partitions.length, checkpointDirPath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The count written at :222 is originalRDD.partitions.length, and the read-back at :230 runs the new check in getPartitions first against that same number. So on the write path the new MISSING_FILES pre-empts CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH at :231, which is now reachable only when _num_partitions was not written or not read back, or the check is disabled. The usual way to hit this is a cluster-mode checkpoint dir on local disk. That case used to get the SPARK-58770 hint that the directory is not on storage both the driver and the executors can reach; now it says trailing files may have been lost and suggests turning the flag off. Doing so only brings back the old error at :231, and leaves detection off.

Moving :222 after the check at :230 fixes this: record N only once the directory is known to hold N part files, so the read-back finds no _num_partitions and skips the new check. A LocalFileSystem subclass whose listStatus omits the last part file can test it by asserting the old error condition.

} {
dos.close()
}
fs.rename(tmpFilePath, countFilePath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reader accepts exactly 13 bytes, so any prefix left by a torn write fails in readFully or readLong with EOFException and ends up where a missing file does: detection is off for that directory. Temp-then-rename only changes whether that read logs a WARN or a debug line. I asked for it in the first round together with the fixed length, but the length alone already exposes a torn write.

Since you asked whether to trim things back, this is the part I would trim: write _num_partitions directly and keep the current WARN on failure, which removes deleteTempFile, the inner try, the renamed branch, and the three rename/cleanup test cases with their FileSystem subclasses. The two remaining injection filesystems would then match _num_partitions instead of the temp name, and the write-failure case would assert that the reader rejects the partial file. Not blocking.


// Must recover normally with all 4 partitions and correct data.
val recovered = sc.checkpointFile[Int](rdd.getCheckpointFile.get)
assert(recovered.partitions.length === 4)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:862 is the backward-compatibility test for directories without the file, but it only checks the partition count and the data. If the FileNotFoundException case at ReliableCheckpointRDD.scala:445 is removed, the exception falls into the NonFatal case at :449, the reader still returns None, and the test stays green, while every read of a pre-upgrade directory logs a "truncation detection will be inactive" WARN with a stack trace.

Wrapping the read in withLogAppender and asserting that WARN is absent would pin it.

This branch has not been deployed

No deployments
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.

3 participants