Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-21372] spark writes one log file even I set the number of spark_rotate_log to 0 #18599

Closed
wants to merge 1 commit into from

Conversation

liu-zhaokun
Copy link
Contributor

https://issues.apache.org/jira/browse/SPARK-21372
Spark writes one log file even I set the number of spark_rotate_log to 0.Meanwhile,the number of spark_rotate_log also can set to a string type and other type.So I think we should make some changes.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@jiangxb1987
Copy link
Contributor

jiangxb1987 commented Jul 11, 2017

Did you see any failure in production environment caused by this issue?

@@ -78,6 +78,12 @@ spark_rotate_log ()
if [ -n "$2" ]; then
num=$2
fi

expr $num + 0 &>/dev/null
if [ $? -ne 0 ] || [ $num -eq 0 ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Are you trying to verify if num is a number? I get it but you would already get a bash error in $num -eq 0 if so and we don't otherwise validate script input that way. Isn't this forcing num to be nonzero, when I thought the issue was handling the case of 0 correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
Yes, we should avoid this scenario that num isn't number.If num isn't a number,num will be set to 5.

@liu-zhaokun
Copy link
Contributor Author

@jiangxb1987
When I want to change the number of log file,I test the scenario that num=0,and spark writes one log file.When I test the scenario that set num to Invaild symbol,the logic is wrong.So I think we should modify it to guarantee this feature.

@jiangxb1987
Copy link
Contributor

jiangxb1987 commented Jul 11, 2017

AFAIK the function spark_rotate_log is never used with num param set, unless you set log param manually, in that case I think you are responsible for the data validation, it would be interesting to see how this issue is triggered, could you share your script please? Thank you!

@liu-zhaokun
Copy link
Contributor Author

@jiangxb1987
I want to change the number of log file ,so I passed num param manually by modify code in this file.In fact,this bug is there,even I don't trigger it,right?Shouldn't we modify it?

@srowen
Copy link
Member

srowen commented Jul 11, 2017

@liu-zhaokun I don't understand what bug you are referring to. We'd need to understand how an unmodified script displays some incorrect behavior.

@liu-zhaokun
Copy link
Contributor Author

@srowen
If I set num to 0,there shouldn't create a logfile or we should make some deal for 0,and If I set num some invaild symbol,the function doesn't work normally,right?I am sorry for my poor English.

@srowen
Copy link
Member

srowen commented Jul 11, 2017

How would you set this with normal operation?
If you change the script to set a value that wasn't expected at that point given how the script works, of course it may fail. That's not a bug.

@@ -78,6 +78,12 @@ spark_rotate_log ()
if [ -n "$2" ]; then
num=$2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
There provide a param,num, to set the number of logfile,but it wasn't used in line 179.Since it doesn't work,can I remove the param "num" and use it just as a variable?

@liu-zhaokun
Copy link
Contributor Author

@srowen
I think spark provides the param,so I can pass it in the script.

@srowen
Copy link
Member

srowen commented Jul 12, 2017

Let's close this. You aren't providing a scenario where an input causes a problem, and I think are still saying the error happens when you modify the script.

@liu-zhaokun
Copy link
Contributor Author

@srowen
The param "num" which defined in function spark_rotate_log() isn't used,if users don't modify the script,it will never be invoked,so could I create a new PR to delete the param and replace it with a fixed number 5?

@srowen srowen mentioned this pull request Jul 31, 2017
@asfgit asfgit closed this in 3a45c7f Aug 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants