You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "chenyu-opensource (via GitHub)" <gi...@apache.org> on 2023/09/14 02:53:42 UTC
[GitHub] [spark] chenyu-opensource opened a new pull request, #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
chenyu-opensource opened a new pull request, #42919:
URL: https://github.com/apache/spark/pull/42919
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
--
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] srowen commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1326687425
##########
docs/configuration.md:
##########
@@ -694,10 +694,10 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""(disabled)</td>
Review Comment:
You didn't fix this one
--
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] HyukjinKwon commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1325299577
##########
docs/configuration.md:
##########
@@ -694,7 +694,7 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""</td>
Review Comment:
Then it seems like `""`. It is disabled by default so I think it's not super confusing?
--
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] chenyu-opensource commented on pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on PR #42919:
URL: https://github.com/apache/spark/pull/42919#issuecomment-1718682598
The default value will take different logic which is between "" and (none).
Please give me a review when you have time. @srowen @LuciferYang
Thank you so much.
--
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] chenyu-opensource commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1325304574
##########
docs/configuration.md:
##########
@@ -694,7 +694,7 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""</td>
Review 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.
--
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] chenyu-opensource commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1326706268
##########
docs/configuration.md:
##########
@@ -694,10 +694,10 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""(disabled)</td>
Review Comment:
@srowen Sorry about that.
I had fix it
--
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] srowen commented on pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on PR #42919:
URL: https://github.com/apache/spark/pull/42919#issuecomment-1722228752
Merged to master
You reused a JIRA, when this should use a new JIRA.
They're really closely related so it's OK here, but you should edit the JIRA to describe the second change you made, and keep separate changes separated in the future
--
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] chenyu-opensource commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1326682581
##########
docs/configuration.md:
##########
@@ -694,13 +694,13 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""(disabled)</td>
<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",
Review Comment:
@srowen Thank you for your advice.
I had follow your advice and adjust
--
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] HyukjinKwon commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1325299577
##########
docs/configuration.md:
##########
@@ -694,7 +694,7 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""</td>
Review Comment:
Then it seems like `""` instead of an empty string. It is disabled by default so I think it's not super confusing?
--
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] srowen commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1325320926
##########
docs/configuration.md:
##########
@@ -694,7 +694,7 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""</td>
Review 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
--
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] chenyu-opensource commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1325398623
##########
docs/configuration.md:
##########
@@ -694,7 +694,7 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""</td>
Review Comment:
@srowen Thank you for your advice.
I had submit a new commit to express the default value and reaffirm the meaning.
--
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] chenyu-opensource commented on pull request #42919: [SPARK-45160][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "chenyu-opensource (via GitHub)" <gi...@apache.org>.
chenyu-opensource commented on PR #42919:
URL: https://github.com/apache/spark/pull/42919#issuecomment-1722644892
@srowen
I had use a new issure.
https://issues.apache.org/jira/browse/SPARK-45160
--
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] srowen commented on a diff in pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #42919:
URL: https://github.com/apache/spark/pull/42919#discussion_r1326202811
##########
docs/configuration.md:
##########
@@ -694,13 +694,13 @@ Apart from these, the following properties are also available, and may be useful
</tr>
<tr>
<td><code>spark.executor.logs.rolling.strategy</code></td>
- <td>(none)</td>
+ <td>""(disabled)</td>
<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",
Review 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.
--
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] srowen closed pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen closed pull request #42919: [SPARK-45146][DOCS]Update the default value of 'spark.executor.logs.rolling.strategy'
URL: https://github.com/apache/spark/pull/42919
--
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