LuciferYang opened a new pull request, #57923: URL: https://github.com/apache/spark/pull/57923
### What changes were proposed in this pull request? This PR converts the four remaining `_LEGACY_ERROR_TEMP_*` conditions in `SparkCoreErrors`' storage and shuffle group, continuing the cleanup under [SPARK-37935](https://issues.apache.org/jira/browse/SPARK-37935). Three get user-facing names; one becomes an internal error. | Legacy | Builder | Now | SQLSTATE | |---|---|---|---| | `_LEGACY_ERROR_TEMP_3033` | `unableToRegisterWithExternalShuffleServerError` | `UNABLE_TO_REGISTER_WITH_EXTERNAL_SHUFFLE_SERVICE` | 58030 | | `_3035` | `shuffleBlockMigrationNotSupportedError` (renamed) | `SHUFFLE_BLOCK_MIGRATION_NOT_SUPPORTED` | 0A000 | | `_3036` | `failToStoreBlockOnBlockManagerError` | `INTERNAL_ERROR_STORAGE` (entry deleted) | XX000 | | `_3037` | `localBlockDataNotFoundError` (renamed) | `LOCAL_BLOCK_DATA_NOT_FOUND` | 58030 | This is the last batch of the `_3021-3042` cluster. `_3028` is left out on purpose: its builder is a bare `<errorMsg>` passthrough whose single call site both duplicates the `taskSet.abort` two lines above it and has its throw swallowed by `Inbox.safelyCall`, so converting it honestly needs a design decision of its own rather than a rename. ### Why `_3036` gets no name It is thrown from the `StreamCallbackWithID.onComplete` that `BlockManager.putBlockDataAsStream` returns, when the block store declines a replica. The condition cannot survive to a reader: `TransportRequestHandler` wraps the exception in a plain `IOException` and answers `RpcFailure` carrying only `stackTraceToString(e)`, and the sender rebuilds that string as a `RuntimeException`. By the time anything logs it, the condition name, SQLSTATE and message parameters are gone — a name would be greppable nowhere, and `checkError` / `getCondition` / SQLSTATE routing would all see a `RuntimeException`. `NettyBlockRpcServer.receive` already reports the same "store declined the block" outcome as an internal error when it handles the non-streaming `UploadBlock` message. Its category is `STORAGE`, not the `NETWORK` that `NettyBlockRpcServer.receive` uses, because the category tracks the package of the throw site — here `org.apache.spark.storage`. Every other `category` argument in `SparkCoreErrors` is `STORAGE`. ### Reachability, per named condition - **`_3033` — operator/environment failure that propagates.** `registerWithExternalShuffleServer` retries `spark.shuffle.registration.maxAttempts` times (default 3); the `case NonFatal(e)` arm normally fires only on the last attempt. Nothing catches it up the chain, so the executor fails to construct. Actionable by whoever runs the shuffle service. - **`_3035` — third-party extension code.** Reached only when a third-party `BlockingShuffleManager` supplies a `ShuffleBlockResolver` that does not mix in `MigratableResolver`, and that executor receives a migrated shuffle block during decommissioning. In-tree managers all resolve to `IndexShuffleBlockResolver`, which does implement it; a manager providing no resolver at all fails earlier with a different error. Per the precedent of `CANNOT_LOAD_CATALOG`, the extension author is the actor who can fix it, so it gets a name. - **`_3037` — environment failure, and the throw is the SPARK-15736 recovery.** A `DiskStore` file vanished under a read lock. `handleLocalReadFailure` de-registers the block so its unavailability reaches the driver, then fails this attempt. It surfaces in the driver's task-failure output at least once per lost block. ### Does this PR introduce _any_ user-facing change? Yes, to error messages — no API change. Converting any legacy condition also adds the `[CONDITION] ` prefix and the ` SQLSTATE: xxxxx` suffix, since `SparkThrowableHelper.formatErrorMessage` suppresses both only for `_LEGACY_ERROR_`-prefixed names. Two of the four had defects beyond the rename: - **`_3035` rendered an identity hash.** It interpolated a `ShuffleBlockResolver` object, and neither that trait nor `IndexShuffleBlockResolver` overrides `toString`, so the message read `org.apache.spark.shuffle.Foo@1a2b3c4d`. It now passes `getClass.getName`. - **`_3033` had a stray space before its colon** (`due to : <message>`), and now falls back to `e.toString` when the cause carries no message — otherwise a message-less throwable rendered the literal `null`. The three new messages deliberately avoid claiming more than the code guarantees, which took several passes to get right: - **No deictic machine reference.** "this executor" is wrong wherever the text crosses an RPC boundary and is read on the peer — `_3037` can be thrown while serving a remote read, and `BlockManager` also runs on the driver. `_3035` says "the receiving executor" rather than "this executor" because the party that reads it for diagnosis is the migration *source*. - **A requirement, not an asserted cause.** `_3035` is raised from a `catch case e: ClassCastException`, and the cast lives in a lazy val — so once initialized, a CCE from inside a third-party `putShuffleBlockAsStream` reaches the same handler. The message says the resolver *must implement* `MigratableResolver` instead of asserting it does not, and the CCE is now carried as the cause so a mislabelled one stays diagnosable. - **No recovery promise.** `_3037` says a later read *must* fetch from a replica or recompute (a necessity, not an availability claim). A sealed `localCheckpoint` block has had its lineage cut and can never be recomputed, so promising recovery would be false there. Its tense is past for the state at read time, since `removeBlock` has already run by the time the message is built. `_3036`'s rendered text changes from `Failure while trying to store block <blockId> on <blockManagerId>.` to the internal-error form; it reaches only logs on both sides of the RPC. ### How was this patch tested? Two existing assertions in `BlockManagerSuite` are tightened to `checkError`, both of which a grep for the condition token would have missed: - "we reject putting blocks when we have the wrong shuffle resolver" pinned only a substring of the message prose, leaving the condition and SQLSTATE unchecked — and it is what forced an edit here, since this PR rewords that message. It now asserts the condition, SQLSTATE `0A000`, both parameters, and that the `ClassCastException` survives as the cause. - `testReadWithLossOfOnDiskFiles` (driving "remove block if a read fails due to missing DiskStore files (SPARK-15736)") was a bare `intercept[SparkException]` that checked nothing but the exception type. It now asserts `LOCAL_BLOCK_DATA_NOT_FOUND` with SQLSTATE `58030`. `_3033`'s three existing assertions (`SPARK-20640` ×2, `SPARK-39647`) read the cause text out of `getMessage`, which is why the `message` parameter is kept alongside `cause` rather than dropped as redundant; all three still pass. A `maxAttempts` parameter was considered and rejected: a non-`Exception` `NonFatal` skips the retry guard and reaches the throw on attempt 1, so "after N attempts" would be false. Ran locally on top of the current master: `SparkThrowableSuite` (34 tests) and `BlockManagerSuite` (130 tests), all passing; `core/compile` clean; the JSON regenerated with `SPARK_GENERATE_GOLDEN_FILES=1` produced no extra diff. Each new assertion was verified to actually bite by temporarily breaking what it checks — both SQLSTATEs and the cause — and watching the test go red. On the SQLSTATE choices: 58030 is nominally "I/O error" but is already Spark's de-facto system-failure code, carrying 11 conditions of which several are not literal I/O; `UNABLE_TO_FETCH_HIVE_TABLES` is the closest precedent for `_3033`. `0A000` for `_3035` follows `PIPELINED_SHUFFLE_UNSUPPORTED` in the same subsystem — the resolver loads fine and merely lacks a capability, which is why `46103` ("unresolved class name", used by `CANNOT_LOAD_CATALOG`) does not fit. `error-states.json` defines an `08xxx` connection class, but no condition in the file uses any of it, and a cleanup PR should not become the first. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8) -- 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]
