dongjoon-hyun commented on PR #58619: URL: https://github.com/apache/spark/pull/58619#issuecomment-5840449059
Thank you for making a PR, @holdenk . I left inline comments. Here is a summary. **Main concerns** These come from the Worker layout change, so they apply to every standalone cluster even when `spark.shuffle.service.requireAppScopedLocalDirs` is `false` (the default). 1. **Predictable directory names**: `<root>/spark-<workerId>/<appId>` replaces the random `spark-<uuid>/executor-<uuid>` names. With the default world-writable `java.io.tmpdir`, another local user can pre-create the directory. Because `Files.createDirectories` accepts an existing directory and the `chmod700` result is ignored, the app dir can end up under a parent directory that user owns. 2. **No cleanup on Worker shutdown**: Switching from `Utils.getOrCreateLocalRootDirs` to `Utils.getConfiguredLocalDirs` drops the shutdown-hook deletion. The local dirs (including shuffle data) of apps that are running when a Worker stops are never removed, and an empty `spark-<workerId>` directory is left behind on every Worker restart. 3. **`appId` is used as-is in paths**: A custom `spark.deploy.appIdPattern` that contains `:` or `/` breaks the `SPARK_EXECUTOR_DIRS` split and the app-scope check. **Suggestion** Keep the existing random root and executor directories, and add `appId` only as a leaf directory, i.e., `<root>/spark-<uuid>/executor-<uuid>/<appId>`. This still satisfies the appId-segment check, keeps the names unpredictable and the shutdown-hook cleanup in place, and removes the need for the per-worker directory and the workerId sanitization. **Documentation** - In standalone mode, `spark.authenticate` does not tie a registration to its appId (all applications share one secret), so "enable both together" is misleading. - The app-scope check accepts the appId as a segment anywhere in the path, which is weaker than the documentation says. - The new layout is not "additive", and the layout in the PR description does not match the implementation. **Tests** - The new `WorkerSuite` test does not go through `LaunchExecutor`. - The e2e test gets the port via reflection and starts the service outside of `try`. -- 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]
