[SPARK-29619][PYTHON][CORE] Add retry times when reading the daemon port. #26282
Conversation
|
Test build #112767 has finished for PR 26282 at commit
|
| daemonPort = in.readInt() | ||
| } catch { | ||
| case _: EOFException => | ||
| throw new SparkException(s"No port number in $daemonModule's stdout") |
There was a problem hiding this comment.
Why don't you just fix exception message or adding an error level log? Seems confusing error doesn't justify retrying
There was a problem hiding this comment.
Adding a retry sometimes makes the task successful.
There was a problem hiding this comment.
port 0 in daemon.py will automatically allocate available socket. In which case does it fail?
There was a problem hiding this comment.
In our production environment , the python sub process sometimes will exit from code 139 .
When I debug it and find the python process is dead.
So , the code in.ReadInt () will throw EOFException not related the port .
There was a problem hiding this comment.
When I debug it and find the python process is dead.
Do you know why it was dead? We might better have to make daemon more robust rather than just retrying.
There was a problem hiding this comment.
Can you tell me in which case it can fail by user code?
There was a problem hiding this comment.
We have a lot job with PySpark. We only find a small number of job causes the issue.
A user tries to catch some exception of Python code, and we will test it.
There was a problem hiding this comment.
@HyukjinKwon If when starting Python process, but exited caused by a unstable factor of system, do we need to make the effort to retry?
There was a problem hiding this comment.
Yes I also don't see a reason to retry, insofar as I don't see evidence that it would make subsequent attempts succeed.
There was a problem hiding this comment.
@srowen Thanks for your review. I will close this PR.
|
Test build #112768 has finished for PR 26282 at commit
|
|
retest this please |
|
Test build #112954 has finished for PR 26282 at commit
|
|
retest this please |
|
Test build #112976 has finished for PR 26282 at commit
|
beliefer
changed the title
beliefer
changed the title
beliefer
changed the title
7 participants
What changes were proposed in this pull request?
This PR is related to #26510 and add retry mechanism to start process.
We have a lot job with PySpark. We only find a small number of job causes the issue. After some try will reduce the issue.
I think the root cause of the exit is the robust problem with the user code.
If when starting Python process, but exited caused by a unstable factor of system, do we need to make the effort to retry?
Why are the changes needed?
In order to clarify the exception and try three times default.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Exists UT.