[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-09 Thread advancedxy 
Github user advancedxy commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160587943
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

> 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...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread zsxwing 
Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160236611
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

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.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/20178


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160173491
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

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.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread advancedxy 
Github user advancedxy commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160170753
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

So, keep it this way or add a constructor and fix Json parser?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160093005
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

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`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-08 Thread zsxwing 
Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160087405
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

It may break the json protocol?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread advancedxy 
Github user advancedxy commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160077159
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

h, maybe.. Let me try it out after work...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160076700
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

hmmm, can we add a new constructor for `attemptId`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160076571
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

ah i see, let's keep it.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread advancedxy 
Github user advancedxy commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160076462
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/20178#discussion_r160076144
  
--- Diff: core/src/main/scala/org/apache/spark/scheduler/StageInfo.scala ---
@@ -56,6 +56,8 @@ class StageInfo(
 completionTime = Some(System.currentTimeMillis)
   }
 
+  def attemptNumber(): Int = attemptId
--- End diff --

how about
```
class StageInfo(
...
val attempNumber: Int,
   ...  {
  @deprecated
  def attempId: Int = attemptNumber
}
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20178: [Spark-22952][CORE] Deprecate stageAttemptId in f...

2018-01-07 Thread advancedxy 
GitHub user advancedxy opened a pull request:

https://github.com/apache/spark/pull/20178

[Spark-22952][CORE] Deprecate stageAttemptId in favour of stageAttemptNumber

## 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


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/advancedxy/spark SPARK-22952

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20178.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20178


commit 4b18ea4f109b0f8d865e3fff8f415d87db528fa5
Author: Xianjin YE 
Date:   2018-01-07T15:17:16Z

Deprecate attemptId and in favour of attemptNumber

commit 2ec919761ced379f00e1fa9804a66e3b15e9d2e9
Author: Xianjin YE 
Date:   2018-01-07T15:43:28Z

Replace usage of stageAttemptId with stageAttemptNumber




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org