dongjoon-hyun commented on PR #58004:
URL: https://github.com/apache/spark/pull/58004#issuecomment-5344861356
I re-reviewed at `a1dd9bf`. Findings #2 (the test hand-building the
`rdd-<id>` path) and #4 (the internal error text) from my earlier review are
addressed, and #3 was partly answered by the rename. Re-verified the mechanical
parts against the new HEAD: the four JSON entries are still at the correct
alphabetical positions (checked the whole key list, not just the neighbors),
the file is still in the generator's format, `55019` and `58030` are both
registered in `error-states.json` and match existing usage (58030 is already
shared by `INVALID_BUCKET_FILE`, `CANNOT_LOAD_STATE_STORE` and 11 others),
every placeholder matches a `messageParameters` key, no
`_LEGACY_ERROR_TEMP_3016`..`3020` reference and no occurrence of the five old
message strings remains anywhere in the repo, `docs/sql-error-conditions.md`
needs no update since it is generated by `build-error-docs.py`, and all 33
check runs on `a1dd9bf` are green.
Two things left, one of which blocks the merge.
**1. Please refresh the PR description before merging — it still describes
the pre-rename state**
`dev/merge_spark_pr.py` uses the PR body as the commit message, so as
written the commit log will name a condition that does not exist. Three spots:
- the table row: `| _3017 | invalidCheckpointFileError |
INVALID_CHECKPOINT_FILE | 58030 |` — the builder is now
`invalidCheckpointDirectoryError` and the condition is
`INVALID_CHECKPOINT_DIRECTORY`
- "`invalidCheckpointFileError` gains an `expectedFileName` parameter"
- the testing section: "asserts `INVALID_CHECKPOINT_FILE` with all three
message parameters"
**2. My earlier finding #1 is still open, and the rename sharpened it**
The new message ends with "The partition files must be numbered contiguously
from part-00000, one per partition", but the check in
`ReliableCheckpointRDD.getPartitions` (line 75-87) does not deliver that in two
directions:
- *Non-numeric `part-*` file.* A stray `part-00000.bak` passes the
`startsWith("part-")` filter, and
`sortBy(_.getName.stripPrefix("part-").toInt)` throws a raw
`NumberFormatException` before the validation loop runs, so the new condition
never fires.
- *Missing trailing file.* Delete `part-00003` from a 4-partition checkpoint
and the remaining names are still contiguous, so the loop passes and
`Array.tabulate(inputFiles.length)` quietly yields a 3-partition RDD. On the
write path `CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH` catches that, but on the
read-back path (`SparkContext.checkpointFile`, which is how streaming recovery
rebuilds `generatedRDDs`) there is no original RDD to compare against and the
data is silently truncated.
Both are pre-existing, but widening the name from `INVALID_CHECKPOINT_FILE`
to `INVALID_CHECKPOINT_DIRECTORY` implies the whole directory is validated,
which makes the gap more visible than before. Either tighten the filter to
`part-\d+` so the first case reaches the new error path (a follow-up is fine),
or trim the last sentence to claim only the contiguity the loop actually checks.
**3. Nit, on the untested condition**
The argument for leaving `CHECKPOINT_RDD_PARTITION_COUNT_MISMATCH` uncovered
— that a fake `FileSystem` would test the fake — reads oddly next to the
`FAILED_CREATE_CHECKPOINT_DIRECTORY` test, which injects the failure exactly
the same way by making `mkdirs` return `false`. A `LocalFileSystem` subclass
whose `listStatus` hides the last `part-*` would exercise the real comparison
in `writeRDDToCheckpointDirectory` (line 178-182), not the fake. Not asking for
the test, just for the reasoning in the description to line up with what the
neighbouring test does.
Otherwise this looks good to me. Splitting `_3017`'s parameters into
(directory, expected name, found name) is a real fix rather than a rename — the
old message pointed at a perfectly valid file — and demoting `_3020` to
`internalError` is right, given that the only construction site of
`ReliableRDDCheckpointData` sits two lines below the
`context.checkpointDir.isEmpty` guard in `RDD.checkpoint()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
