[Spark-22952][CORE] Deprecate stageAttemptId in favour of stageAttemptNumber #20178
Conversation
|
cc @cloud-fan @zsxwing and @squito I only included the changes related to StageInfo's deprecated getter. It would involve too much changes if we want to replace attemptId with attemptNumber everywhere, which I think we should do that until we are targeting Spark 3.0. I even think some of the changes in this patch may not be applied as mixed |
|
Please add me to whitelist... |
|
add to whitelist |
|
ok to test |
| @@ -56,6 +56,8 @@ class StageInfo( | |||
| completionTime = Some(System.currentTimeMillis) | |||
| } | |||
|
|
|||
| def attemptNumber(): Int = attemptId | |||
There was a problem hiding this comment.
how about
class StageInfo(
...
val attempNumber: Int,
... {
@deprecated
def attempId: Int = attemptNumber
}
There was a problem hiding this comment.
If we go this way, I believe it would break source compatibility for Developer API.
Code like new StageInfo(stageId = xx, attemptId = yy, ...) couldn't by compiled any more.
Not sure about binary compatibility
There was a problem hiding this comment.
hmmm, can we add a new constructor for attemptId?
There was a problem hiding this comment.
hmmmm, maybe.. Let me try it out after work...
There was a problem hiding this comment.
So we do not fully deprecate it, it's still attemptId in json. Shall we fix the json too? We may need to add some logic at the parser side to recognize attemptId
There was a problem hiding this comment.
So, keep it this way or add a constructor and fix Json parser?
There was a problem hiding this comment.
I'd like to add a constructor and fix the json parser, otherwise we are not fully deprecating it. But let's do it in a new PR, as it may need quite a lot of changes.
There was a problem hiding this comment.
Oh, I was wrong. It seems the rest api doesn't use this class. It's using org.apache.spark.status.api.v1.StageData. However, it's also called attemptId in this class.
There was a problem hiding this comment.
But let's do it in a new PR, as it may need quite a lot of changes.
Lets's do that after Spark 2.3 release then?
However, it's also called attemptId in this class.
Yeah, (stage)attemptId is over a lot of places...
|
Test build #85785 has finished for PR 20178 at commit
|
|
thanks, merging to master/2.3! |
…tNumber ## What changes were proposed in this pull request? 1. Deprecate attemptId in StageInfo and add `def attemptNumber() = attemptId` 2. Replace usage of stageAttemptId with stageAttemptNumber ## How was this patch tested? I manually checked the compiler warning info Author: Xianjin YE <[email protected]> Closes #20178 from advancedxy/SPARK-22952. (cherry picked from commit 40b983c) Signed-off-by: Wenchen Fan <[email protected]>
What changes were proposed in this pull request?
def attemptNumber() = attemptIdHow was this patch tested?
I manually checked the compiler warning info