[SPARK-45160][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'#42919
[SPARK-45160][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'#42919chenyu0513 wants to merge 4 commits intoapache:masterfrom
Conversation
|
The default value will take different logic which is between "" and (none). |
docs/configuration.md
Outdated
| <tr> | ||
| <td><code>spark.executor.logs.rolling.strategy</code></td> | ||
| <td>(none)</td> | ||
| <td>""</td> |
There was a problem hiding this comment.
Then it seems like "" instead of an empty string. It is disabled by default so I think it's not super confusing?
There was a problem hiding this comment.
@HyukjinKwon Thank you so much for your review.
I think it is unusual. it should be different from other situations, jush like which uses Nil on it's default value. This is just my opinion. Just like you said, it's not super confusing.
Thank you again. Please decide whether to support this pr.
There was a problem hiding this comment.
You could write "" (disabled) to try to both express the default value and reaffirm what it means. But it already says the default is disabled and (none) communicates "not set to anything". I would leave it
There was a problem hiding this comment.
@srowen Thank you for your advice.
I had submit a new commit to express the default value and reaffirm the meaning.
docs/configuration.md
Outdated
| <td> | ||
| Set the strategy of rolling of executor logs. By default it is disabled. It can | ||
| be set to "time" (time-based rolling) or "size" (size-based rolling). For "time", | ||
| be set to "time" (time-based rolling) or "size" (size-based rolling) or ""(empty string). For "time", |
There was a problem hiding this comment.
Please put space after quotes here and above. (empty string) isn't helpful, (disabled) is. Then you can remove the sentence at the end that you added.
There was a problem hiding this comment.
@srowen Thank you for your advice.
I had follow your advice and adjust
docs/configuration.md
Outdated
| <tr> | ||
| <td><code>spark.executor.logs.rolling.strategy</code></td> | ||
| <td>(none)</td> | ||
| <td>""(disabled)</td> |
|
Merged to master |
chenyu0513
changed the title
|
@srowen |
3 participants
What changes were proposed in this pull request?
The PR updates the default value of 'spark.executor.logs.rolling.strategy in configuration.html on the website
Why are the changes needed?
The default value of 'spark.executor.logs.rolling.strategy' is '', but is not (none). The website has Inaccurate description.It can bring ambiguity.
Does this PR introduce any user-facing change?
No
How was this patch tested?
It doesn't need to.
Was this patch authored or co-authored using generative AI tooling?
No