LuciferYang opened a new pull request, #57663: URL: https://github.com/apache/spark/pull/57663
### What changes were proposed in this pull request? This PR converts 11 legacy error conditions in `SparkCoreErrors` that guard **engine-internal invariants** into `SparkException.internalError(...)`, and deletes their entries from `error-conditions.json`. Per the error-conditions [README](https://github.com/apache/spark/blob/master/common/utils/src/main/resources/error/README.md), a throw site that a correct Spark cannot reach from user code should become an internal error rather than being given a user-facing name. | Legacy | Builder | Now | |---|---|---| | `_LEGACY_ERROR_TEMP_3024` | `accessNonExistentAccumulatorError` | `INTERNAL_ERROR` | | `_3025` | `sendResubmittedTaskStatusForShuffleMapStagesOnlyError` | `INTERNAL_ERROR` | | `_3030` | `taskHasNotLockedBlockError` | `INTERNAL_ERROR_STORAGE` | | `_3031` | `blockDoesNotExistError` | `INTERNAL_ERROR_STORAGE` | | `_3032` | `waitingForReplicationToFinishError` | `INTERNAL_ERROR` (keeps `cause`) | | `_3034` | `waitingForAsyncReregistrationError` | `INTERNAL_ERROR` (keeps `cause`) | | `_3038` | `failToGetBlockWithLockError` | `INTERNAL_ERROR_STORAGE` | | `_3039` | `blockStatusQueryReturnedNullError` | `INTERNAL_ERROR_STORAGE` | | `_3040` | `unexpectedBlockManagerMasterEndpointResultError` | `INTERNAL_ERROR_STORAGE` | | `_3042` | `failToGetNonShuffleBlockError` | `INTERNAL_ERROR_STORAGE` (keeps `cause`) | | `_3041` | `unsupportedOperationError` (deleted) | `UNSUPPORTED_CALL.WITHOUT_SUGGESTION` | Message text is preserved for all of them (a trailing period was added where missing). Two entries get more than a mechanical rewrite: - **`_3041` had an empty message** (`"message": [""]`), so it rendered as the empty string. Its builder is deleted and `DiskBlockObjectWriter.write(b: Int)` — an `OutputStream` method the class does not implement — now throws the existing `UNSUPPORTED_CALL.WITHOUT_SUGGESTION` with explicit `className`/`methodName`. The no-arg `SparkUnsupportedOperationException()` helper was tried first and rejected: it derives those from `stackTrace(3)`, which for a plain Scala class method resolves to the *caller's* frame (measured: `methodName` came out as `$anonfun$new$3`). - **`_3040` had no message parameters at all** ("BlockManagerMasterEndpoint returned false, expected true."), so the log never said which RPC failed. Its builder gains a `message: Any` parameter. `BlockManagerMaster.tell` is `private` with two call sites, passing `RemoveExecutor(execId)` and `StopBlockManagerMaster`; both are a case class/object whose `toString` carries no host, credential, or block content (`BlockManagerId` never reaches `tell`). `_3042` uses `new SparkException("INTERNAL_ERROR_STORAGE", ..., cause = e)` directly because `SparkException.internalError` has no overload accepting both a `cause` and a `category` (precedent: `ExecutorDeadException`). `_3032`/`_3034` keep bare `INTERNAL_ERROR` — they are dead branches (see below), so the category buys nothing, and adding the missing overload to `common/utils` would be unrelated churn here. ### Reachability, per condition The classification is the substance of this PR, so it is spelled out rather than asserted: - **Unreachable by construction.** `_3025` — the only producer of `Resubmitted` is `TaskSetManager.executorLost`, gated on `isShuffleMapTasks`, so the non-`ShuffleMapStage` branch cannot be taken. `_3039` — `Future.sequence` cannot yield `null`; the check is a vestige of a pre-`awaitResult` rewrite. `_3040` — both `tell` handlers reply `true` unconditionally; an endpoint failure throws out of `askSync` instead. `_3042` — every `blockId` reaching `throwFetchFailedException` comes from map-output-derived shuffle ids, so the defensive `case _` is unreachable. `_3030` — both callers run on the thread that acquired the write lock. `_3041` — `write(b: Int)` has no caller in tree. - **Dead catch blocks.** `_3032` and `_3034` wrap only a `ThreadUtils.awaitReady(future, Duration.Inf)` call in `catch NonFatal`. `Awaitable.ready` discards the `Try` and never surfaces the future's failure, and `Duration.Inf` takes the `acquireSharedInterruptibly` path so no timeout is possible — nothing can reach either catch. (`InterruptedException` can escape `awaitReady`, but it is not `NonFatal`, so it passes through untouched both before and after this change.) `_3034`'s enclosing method is additionally documented "For testing." with only test callers. - **Reachable only by a benign race, and swallowed.** `_3024` is *not* strictly unreachable: `AccumulatorContext.get` holds `WeakReference`s and `ContextCleaner.doCleanupAccum` removes entries, so an in-flight `CompletionEvent` can carry an id that has already been collected or cleaned. The throw is caught by a `catch NonFatal` in the *same* method (`DAGScheduler.updateAccumulators`), which only logs — it never escapes to a job or a user. There is no user-visible exception to name, so `internalError` is the right target, but the branch should not be described as unreachable. - **Reachable only during shutdown.** `_3031` and `_3038` are guarded by lock ownership at every caller, with one residual path: `BlockInfoManager.clear()` uses `tryLock` and drops entries without honoring existing holders, so an executor stopping while tasks are still finishing can reach them. That exception is itself suppressed by `Executor`'s `case t: Throwable if env.isStopped` and never reaches the driver. The remaining 10 conditions in the `_3021-3042` range are **intentionally left alone** — they are operational or user-reachable failures that deserve proper names in a follow-up, and labeling them `INTERNAL_ERROR` would report real failures as Spark bugs: `_3021`/`_3022` (RPC races during executor/endpoint shutdown), `_3023` (`submitMapStage` on a 0-partition RDD — user API misuse), `_3026` (`TaskInfo.duration` on an unfinished task — `TaskInfo` is `@DeveloperApi`), `_3028` (barrier stage got partial offers under a legacy config), `_3029` (the standalone Master rejected the application), `_3033` (external shuffle service registration exhausted its retries), `_3035` (a third-party `ShuffleManager` whose resolver is not `MigratableResolver`), `_3036` (a replica genuinely failed to store), `_3037` (a `DiskStore` file vanished — the SPARK-15736 recovery path). The ten converted builders remain thin wrappers in `SparkCoreErrors` rather than being inlined at their throw sites. Inlining would match what most of `core` does for internal errors, but it would enlarge the diff across five more files for no behavior change; `_3041` is inlined only because its builder became empty. ### Why are the changes needed? The error-conditions README disallows new `_LEGACY_ERROR_TEMP_*` entries and asks existing ones to be resolved. This clears 11 of them, under the umbrella [SPARK-37935](https://issues.apache.org/jira/browse/SPARK-37935). Two of the 11 were also defective in their own right: `_3041`'s message was empty and `_3040`'s carried no parameters. ### Does this PR introduce _any_ user-facing change? No new user-facing API, but one behavior change worth a reviewer's explicit sign-off. `DAGScheduler.abortStage` passes a task exception straight to the user only when `getCondition != null && !isInternalError(condition)`. The old `_LEGACY_ERROR_TEMP_*` names passed that filter; `INTERNAL_ERROR*` does not. So for the four conditions thrown inside tasks on executors — `_3030`, `_3031`, `_3038`, `_3042` — a job that fails through them (after the task retries to `maxTaskFailures`) now reports `SparkException: Job aborted due to stage failure: ...` wrapping the original, instead of the original directly, and gains SQLSTATE `XX000`. That is the intended consequence of labeling these as internal errors, and no test asserts the old unwrapped shape. `_3041`'s rendered message changes from the empty string to `Cannot call the method "write" of the class "org.apache.spark.storage.DiskBlockObjectWriter".`, and its exception type stays a subclass of `UnsupportedOperationException`. ### How was this patch tested? - `SparkThrowableSuite` — validates that the JSON is well-formed, key-sorted, and round-trips after the 11 deletions. - `BlockInfoManagerSuite` — its "removing a non-existent block" test was tightened from a bare `intercept[SparkException]` to `checkError` on `INTERNAL_ERROR_STORAGE`, so the conversion is now pinned by an assertion rather than passing vacuously. - `DiskBlockObjectWriterSuite` — new test asserting `UNSUPPORTED_CALL.WITHOUT_SUGGESTION` with the expected `className`/`methodName`. - `BlockManagerMasterSuite` — covers the `_3040` signature change. All four suites pass (70 tests) on top of the current master, and `core/compile` is clean. No new tests were added for the unreachable conversions, since there is no way to trigger them. ### 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]
