Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented May 19, 2020

What changes were proposed in this pull request?

in spark-daemon.sh, spark_rotate_log() accepts $2 as a custom setting for the number of maximum rotate log files, but this part of code is actually never used.

This PR adds SPARK_LOG_MAX_FILES environment variable to represent the maximum log files of Spark daemons can rotate to.

Why are the changes needed?

the logs files that all spark daemons are hardcoded as 5, but it supposed to be configurable

Does this PR introduce any user-facing change?

yes, SPARK_LOG_MAX_FILES is added to represent the maximum log files of Spark daemons can rotate to.

How was this patch tested?

verify locally for the added shell script:

 kentyao@hulk  ~  SPARK_LOG_MAX_FILES=1 sh test.sh
1
 kentyao@hulk  ~  SPARK_LOG_MAX_FILES=a sh test.sh
Error: SPARK_LOG_MAX_FILES must be a postive number
 ✘ kentyao@hulk  ~  SPARK_LOG_MAX_FILES=b sh test.sh
Error: SPARK_LOG_MAX_FILES must be a postive number
 ✘ kentyao@hulk  ~  SPARK_LOG_MAX_FILES=-1 sh test.sh
Error: SPARK_LOG_MAX_FILES must be a postive number
 ✘ kentyao@hulk  ~  sh test.sh
5
 ✘ kentyao@hulk  ~  cat test.sh
#!/bin/bash

if [[ -z ${SPARK_LOG_MAX_FILES} ]] ; then
      num=5
elif [[ ${SPARK_LOG_MAX_FILES} -gt 0 ]]; then
      num=${SPARK_LOG_MAX_FILES}
else
    echo "Error: SPARK_LOG_MAX_FILES must be a postive number"
    exit -1
fi

@yaooqinn
Copy link
Member Author

@SparkQA
Copy link

SparkQA commented May 19, 2020

Test build #122845 has finished for PR 28580 at commit 311d0f3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just to make doubly sure, can you update how you tested in the PR description?

@yaooqinn
Copy link
Member Author

@HyukjinKwon updated. thanks for approving

@HyukjinKwon
Copy link
Member

Let me merge it. There's no test for this script in PR builders.

@HyukjinKwon
Copy link
Member

Merged to master.

@SparkQA
Copy link

SparkQA commented May 20, 2020

Test build #122890 has finished for PR 28580 at commit a54c6bd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants