[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[HyukjinKwon]

Github user HyukjinKwon commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136957034
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

I am a committer. Generally, a PR should targets an issue without other 
changes. I use common sense - fixing styles around the changes is fine if it 
does not look making revert/backport harder.


---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[tejasapatil]

Github user tejasapatil commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136892336
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

not a committer but would like to leave this suggestion : 
- codestyle changes are orthogonal to the motive of the PR and should be 
done separately. Generally, every PR should address one problem and not have 
changes unrelated to it. In event of revert or bisecting commits to pin-point 
regression, following this practice helps a lot.
- It would be beneficial to see why checkstyle does not catch such 
instances and fix that (along with making all such instances consistent with 
the rules). Otherwise this would be a one off fix and we would continue to pile 
up similar inconsistencies in future development without anyone realising this.


---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[dongjoon-hyun]

Github user dongjoon-hyun commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136752724
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

Sorry, I'm not in a position to say that. Maybe, committers will review 
this and give a direction.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136750244
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

Should I fix the other places then?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[dongjoon-hyun]

Github user dongjoon-hyun commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136726682
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -177,11 +179,11 @@ class SourceProgress protected[sql](
 }
 
 ("description" -> JString(description)) ~
-  ("startOffset" -> tryParse(startOffset)) ~
-  ("endOffset" -> tryParse(endOffset)) ~
-  ("numInputRows" -> JInt(numInputRows)) ~
-  ("inputRowsPerSecond" -> safeDoubleToJValue(inputRowsPerSecond)) ~
-  ("processedRowsPerSecond" -> 
safeDoubleToJValue(processedRowsPerSecond))
--- End diff --

Yep. I guess that those are the less frequent style.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[dongjoon-hyun]

Github user dongjoon-hyun commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136726519
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

Yes. Please refer here, https://github.com/databricks/scala-style-guide
```scala
class Foo(
val param1: String,  // 4 space indent for parameters
val param2: String,
val param3: Array[Byte])
  extends FooInterface  // 2 space indent here
  with Logging {

  def firstMethod(): Unit = { ... }  // blank line above
}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136726445
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -177,11 +179,11 @@ class SourceProgress protected[sql](
 }
 
 ("description" -> JString(description)) ~
-  ("startOffset" -> tryParse(startOffset)) ~
-  ("endOffset" -> tryParse(endOffset)) ~
-  ("numInputRows" -> JInt(numInputRows)) ~
-  ("inputRowsPerSecond" -> safeDoubleToJValue(inputRowsPerSecond)) ~
-  ("processedRowsPerSecond" -> 
safeDoubleToJValue(processedRowsPerSecond))
--- End diff --

I though it'd been the opposite - see 
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala#L53-L57
 and 
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala#L128-L140.

I've made the section to match the style of the others in the source file.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136726289
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

Really?! Then the other places in the file are incorrect like 
https://github.com/jaceklaskowski/spark/blob/337ad489bb43ef93a651bdd4952bd7f0738698dc/sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala#L90
 or 
https://github.com/jaceklaskowski/spark/blob/337ad489bb43ef93a651bdd4952bd7f0738698dc/sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala#L161?
 I might be missing something though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[dongjoon-hyun]

Github user dongjoon-hyun commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136724364
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -177,11 +179,11 @@ class SourceProgress protected[sql](
 }
 
 ("description" -> JString(description)) ~
-  ("startOffset" -> tryParse(startOffset)) ~
-  ("endOffset" -> tryParse(endOffset)) ~
-  ("numInputRows" -> JInt(numInputRows)) ~
-  ("inputRowsPerSecond" -> safeDoubleToJValue(inputRowsPerSecond)) ~
-  ("processedRowsPerSecond" -> 
safeDoubleToJValue(processedRowsPerSecond))
--- End diff --

nit, the original style seems to be more frequent and looks correct, 
doesn't it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[dongjoon-hyun]

Github user dongjoon-hyun commented on a diff in the pull request:

https://github.com/apache/spark/pull/19112#discussion_r136724317
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala ---
@@ -200,7 +202,7 @@ class SourceProgress protected[sql](
  */
 @InterfaceStability.Evolving
 class SinkProgress protected[sql](
-val description: String) extends Serializable {
--- End diff --

Hi, @jaceklaskowski . For this, the original one looks correct for me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...

[jaceklaskowski]

GitHub user jaceklaskowski opened a pull request:

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

[SPARK-21901][SS] Define toString for StateOperatorProgress

## What changes were proposed in this pull request?

Just `StateOperatorProgress.toString` + few formatting fixes

## How was this patch tested?

Local build. Waiting for OK from Jenkins.

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

$ git pull https://github.com/jaceklaskowski/spark 
SPARK-21901-StateOperatorProgress-toString

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

https://github.com/apache/spark/pull/19112.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 #19112


commit 337ad489bb43ef93a651bdd4952bd7f0738698dc
Author: Jacek Laskowski 
Date:   2017-09-03T18:17:45Z

[SPARK-21901][SS] Define toString for StateOperatorProgress




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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