[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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