LuciferYang commented on PR #57879: URL: https://github.com/apache/spark/pull/57879#issuecomment-5235624790
> +1, LGTM. I verified the changes against the codebase: > > * All three JSON insertions (`CLUSTER_MANAGER_APPLICATION_FAILURE`, `SCHEDULER_BACKEND_SHUTDOWN_FAILED`, `UNSUPPORTED_CALL.TASK_NOT_FINISHED`) are correctly placed alphabetically, and all three SQLSTATEs exist in `error-states.json` with reasonable precedents (56000 already used by `CHECKPOINT_RDD_BLOCK_ID_NOT_FOUND`, 58030 by 11 conditions). > * `durationCalledOnUnfinishedTaskError` has exactly one caller, so the signature change is safe, and no stray references to the five removed legacy IDs remain anywhere in the repo. > * `stopExecutors()` is only called from `stop()`, so the "later shutdown steps were skipped" clause in the `EXECUTORS` message is accurate. The base-message-ending-with-colon style has plenty of precedent (`DATATYPE_MISMATCH`, `FAILED_JDBC`, etc.). > * Exception types are unchanged, so there is no user-facing API impact and no MiMa concern. > * The new tests reuse existing suite plumbing correctly (the mocked `driverEndpointRef` pattern in the k8s suite, `failedTaskSet`/`failedTaskSetReason` in `TaskSchedulerImplSuite`). > > One minor nit: the title says `_LEGACY_ERROR_TEMP_3021-3029`, but `_3028` is intentionally left for a follow-up. Stating the exact set (`3021-3023,3026,3029`) would make the commit log more precise. The pr title has been rewritten. -- 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]
