You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "j1wonpark (via GitHub)" <gi...@apache.org> on 2026/03/28 23:16:44 UTC
[PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
j1wonpark opened a new pull request, #597:
URL: https://github.com/apache/spark-kubernetes-operator/pull/597
### What changes were proposed in this pull request?
- Use try-with-resources for `OutputStreamWriter` in `createLocalFileForPodTemplateSpec` to
guarantee streams are closed even when an exception is thrown during the write operation.
- Replace the `"UTF-8"` string literal with `StandardCharsets.UTF_8` for type safety.
- Narrow the `catch (Throwable t)` clause in `deleteLocalFileFromPathKey` to
`catch (SecurityException e)`, which is the only exception `File.delete()` can throw.
### Why are the changes needed?
`FileOutputStream` and `OutputStreamWriter` were opened without try-with-resources.
If `writer.write()` or `ModelUtils.asJsonString()` threw an exception, the streams would
remain open, causing file descriptor leaks. Over time this could exhaust the OS file
descriptor limit in long-running operator deployments.
Catching `Throwable` in `deleteLocalFileFromPathKey` is an anti-pattern that swallows
JVM-level errors such as `OutOfMemoryError` and `VirtualMachineError`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Added `testBuildResourceSpecWritesAndCleansTempFilesForPodTemplateSpec` to
`SparkAppResourceSpecFactoryTest` to verify that temp files created for driver and executor
pod template specs are deleted after `buildResourceSpec` completes.
- Ran `./gradlew :spark-operator:test`, `checkstyleMain`, `checkstyleTest`, `pmdMain`,
`spotbugsMain`, and `spotlessCheck` — all passed.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #597: [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory
URL: https://github.com/apache/spark-kubernetes-operator/pull/597
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on PR #597:
URL: https://github.com/apache/spark-kubernetes-operator/pull/597#issuecomment-4173521087
Thank you for the guidance, @dongjoon-hyun.
I removed the misleading test case (`testBuildResourceSpecWritesAndCleansTempFilesForPodTemplateSpec`) since it passes with or without the patch and does not protect against regression.
This PR now contains only the `try-with-resources` improvement and related cleanup (`StandardCharsets.UTF_8`, narrowed `catch` clause).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #597:
URL: https://github.com/apache/spark-kubernetes-operator/pull/597#issuecomment-4201170167
Merged to main.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #597:
URL: https://github.com/apache/spark-kubernetes-operator/pull/597#issuecomment-4163692276
In the community, we remove the misleading test case in that case, @j1wonpark .
For example, we handle independent themes as two PRs.
- We can accept your `try-with-resource` improvement as a better practice without a test case since it's a trivial Java recommendation.
- We can accept your happy scenario test case as a new test improvement independently (if it's needed).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56276] Fix file descriptor leak in SparkAppResourceSpecFactory [spark-kubernetes-operator]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on PR #597:
URL: https://github.com/apache/spark-kubernetes-operator/pull/597#issuecomment-4160513949
Thank you for the feedback. I tried writing a test that fails without the patch, but since `OutputStreamWriter` is created inside the private method, there's no way to inject a spy to verify `close()` is called on the exception path. Restructuring the production code solely for testability felt like over-engineering for this change.
Would it be acceptable to proceed without an additional test for this, or do you have suggestions on how to approach this?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org