You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/13 03:39:57 UTC
[GitHub] [spark] warrenzhu25 opened a new pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
warrenzhu25 opened a new pull request #35501:
URL: https://github.com/apache/spark/pull/35501
### What changes were proposed in this pull request?
Support write container stdout/stderr to file
### Why are the changes needed?
If users want to sidecar logging agent to send stdout/stderr to external log storage, only way is to change entrypoint.sh, which might break compatibility with community version.
### Does this PR introduce _any_ user-facing change?
Yes. User can enable this feature by spark config.
### How was this patch tested?
Added UT in BasicDriverFeatureStepSuite and BasicExecutorFeatureStepSuite
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039738990
In addition, the following is totally different. Users are able to control the shuffle data lifecycle programmatically while this PR doesn't allow any option.
> This is similar issue as shuffle output, which also might run out of disk.
--
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
[GitHub] [spark] martin-g commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039928824
One can use external tools like [logrotate](https://linux.die.net/man/8/logrotate) for 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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35501:
URL: https://github.com/apache/spark/pull/35501#discussion_r806291258
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##########
@@ -82,7 +82,14 @@ private[spark] class BasicDriverFeatureStep(conf: KubernetesDriverConf)
.withName(env._1)
.withValue(env._2)
.build()
- }
+ } ++ {
+ if (conf.get(KUBERNETES_LOG_TO_FILE)) {
Review comment:
Indentation?
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala
##########
@@ -177,7 +177,14 @@ private[spark] class BasicExecutorFeatureStep(
.withValue(opt)
.build()
}
- }
+ } ++ {
+ if (kubernetesConf.get(KUBERNETES_LOG_TO_FILE)) {
Review comment:
ditto.
--
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35501:
URL: https://github.com/apache/spark/pull/35501#discussion_r806291258
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##########
@@ -82,7 +82,14 @@ private[spark] class BasicDriverFeatureStep(conf: KubernetesDriverConf)
.withName(env._1)
.withValue(env._2)
.build()
- }
+ } ++ {
+ if (conf.get(KUBERNETES_LOG_TO_FILE)) {
Review comment:
Indentation?
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala
##########
@@ -177,7 +177,14 @@ private[spark] class BasicExecutorFeatureStep(
.withValue(opt)
.build()
}
- }
+ } ++ {
+ if (kubernetesConf.get(KUBERNETES_LOG_TO_FILE)) {
Review comment:
ditto.
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala
##########
@@ -441,6 +441,19 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter {
))
}
+ test("container log configs") {
Review comment:
Could you add a JIRA ID prefix like
```
- test("container log configs") {
+ test("SPARK-36793: container log configs") {
```
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039738990
In addition, the following is totally different. Users are able to control the shuffle data lifecycle programmatically while this PR doesn't allow any option.
> This is similar issue as shuffle output, which also might run out of disk.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1040764416
@warrenzhu25 I'm going to close this PR because this level of implementation is too primitive and can be achieved via the custom `entrypoint.sh` file when the user builds their docker image. Please feel free to reopen this when there is an update on the proposal.
--
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
[GitHub] [spark] dongjoon-hyun closed pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35501:
URL: https://github.com/apache/spark/pull/35501
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1038816125
Can one of the admins verify this patch?
--
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
[GitHub] [spark] warrenzhu25 commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039721538
> Thank you for making a PR, @warrenzhu25 . However, without a rolling feature, this will kill the executor due to OutOfDisk.
@dongjoon-hyun thanks for reviewing this. For log rolling feature, could we support this in the future? I can create a jira ticket to track this. This is similar issue as shuffle output, which also might run out of disk.
--
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
[GitHub] [spark] warrenzhu25 commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039721538
> Thank you for making a PR, @warrenzhu25 . However, without a rolling feature, this will kill the executor due to OutOfDisk.
@dongjoon-hyun thanks for reviewing this. For log rolling feature, could we support this in the future? I can create a jira ticket to track this. This is similar issue as shuffle output, which also might run out of disk.
--
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
[GitHub] [spark] martin-g commented on a change in pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35501:
URL: https://github.com/apache/spark/pull/35501#discussion_r805650037
##########
File path: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh
##########
@@ -107,4 +107,14 @@ case "$1" in
esac
# Execute the container CMD under tini for better hygiene
-exec /usr/bin/tini -s -- "${CMD[@]}"
+if [ -z "$K8S_SPARK_LOG_PATH" ]; then
Review comment:
The env var has been renamed to `SPARK_K8S_LOG_PATH`
--
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35501:
URL: https://github.com/apache/spark/pull/35501#discussion_r806291920
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala
##########
@@ -441,6 +441,19 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter {
))
}
+ test("container log configs") {
Review comment:
Could you add a JIRA ID prefix like
```
- test("container log configs") {
+ test("SPARK-36793: container log configs") {
```
--
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
[GitHub] [spark] martin-g commented on pull request #35501: [SPARK-36793][K8S] Support write container stdout/stderr to file
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35501:
URL: https://github.com/apache/spark/pull/35501#issuecomment-1039928824
One can use external tools like [logrotate](https://linux.die.net/man/8/logrotate) for 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