dongjoon-hyun commented on PR #58004:
URL: https://github.com/apache/spark/pull/58004#issuecomment-5325633401
I reviewed this PR. Overall it looks solid: the four JSON entries are
inserted at the correct alphabetical positions, both SQLSTATEs (55019, 58030)
are registered in `error-states.json`, every message placeholder matches the
Scala `messageParameters` keys, no references to
`_LEGACY_ERROR_TEMP_3016`..`3020` remain anywhere in the repo, and no MiMa
exclusion is needed since `SparkCoreErrors` is `private[spark]`. I also
verified the new tests' path-string assertions hold (the `listStatus`-qualified
parent matches the qualified checkpoint dir exactly, including on macOS `/var`
vs `/private/var`).
A few findings, in decreasing order of importance:
**1. `getPartitions` can throw a raw `NumberFormatException` before the new
`INVALID_CHECKPOINT_FILE` fires** (pre-existing, but surfaced by this change)
In `ReliableCheckpointRDD.scala` (line 78), a stray non-numeric `part-*`
file, e.g. a leftover `part-00000.bak`, passes the `startsWith("part-")`
filter, and `sortBy(_.getName.stripPrefix("part-").toInt)` throws
`java.lang.NumberFormatException` before the validation loop this PR modifies
can raise the new condition. The new message states "The partition files must
be numbered contiguously from part-00000", which advertises validation the code
cannot deliver for that input. Consider tightening the filter (e.g. to names
matching `part-\d+`) so such files reach the new error path instead, either
here or as a follow-up.
**2. The new test re-implements the `rdd-<id>` path convention**
In the `"checkpoint path that cannot be created"` test, `new
Path(sc.getCheckpointDir.get, s"rdd-${rdd.id}")` hand-builds the per-RDD
directory. `ReliableRDDCheckpointData.checkpointPath(sc, rdd.id).get` is
accessible from the test's package, yields the identical string, and ties the
assertion to the same code path production uses, so a future layout change
cannot silently strand the expectation. (The `startsWith("rdd-")` check inside
`MkdirsFailingFilesystem` cannot use the helper since Hadoop instantiates it
reflectively, so that one hardcode is unavoidable.)
**3. Nit: namespace of the new names**
`CHECKPOINT_DIRECTORY_NOT_SET`, `INVALID_CHECKPOINT_FILE`, and
`FAILED_CREATE_CHECKPOINT_DIRECTORY` claim generic checkpoint names for
RDD-specific errors, while the streaming analog `_LEGACY_ERROR_TEMP_1298`
("checkpointLocation must be specified ...") is still awaiting a name and
condition names are frozen once shipped. Precedent cuts both ways
(`CHECKPOINT_RDD_BLOCK_ID_NOT_FOUND` is RDD-side with a generic prefix;
streaming uses `STREAMING_CHECKPOINT_*`), so this is only a naming-foresight
consideration, and the streaming case would likely be named
`STREAMING_CHECKPOINT_LOCATION_NOT_SET` anyway.
**4. Nit: the internal error text keeps the legacy user-directive phrasing**
`SparkException.internalError("Checkpoint dir must be specified.")` reads as
an instruction to the user, but this branch only fires on a broken invariant.
Other `internalError` sites in core describe the anomalous state ("memory store
not initialized yet", "Index file is deleted already."), so something like
"SparkContext.checkpointDir is unset when creating ReliableRDDCheckpointData"
would give a bug report actual diagnostic content.
Generated-by: Claude Fable 5
--
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]
