You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jaceklaskowski <gi...@git.apache.org> on 2017/09/03 18:21:25 UTC
[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 <ja...@japila.pl>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by dongjoon-hyun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by dongjoon-hyun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81363 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81363/testReport)** for PR 19112 at commit [`337ad48`](https://github.com/apache/spark/commit/337ad489bb43ef93a651bdd4952bd7f0738698dc).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81406 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81406/testReport)** for PR 19112 at commit [`0e2b9c6`](https://github.com/apache/spark/commit/0e2b9c6006593b14b9bda652225252ed475ebbfe).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81406/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by jaceklaskowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by jaceklaskowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81406 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81406/testReport)** for PR 19112 at commit [`0e2b9c6`](https://github.com/apache/spark/commit/0e2b9c6006593b14b9bda652225252ed475ebbfe).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81407 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81407/testReport)** for PR 19112 at commit [`673e088`](https://github.com/apache/spark/commit/673e0880c4752b8401f00f122038cb71274ecaea).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by jaceklaskowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by dongjoon-hyun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81407/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81407 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81407/testReport)** for PR 19112 at commit [`673e088`](https://github.com/apache/spark/commit/673e0880c4752b8401f00f122038cb71274ecaea).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81363/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by dongjoon-hyun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19112
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the issue:
https://github.com/apache/spark/pull/19112
Hey @HyukjinKwon, as the only committer who's been involved in this PR, could you review it again and possibly merge to master? Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by HyukjinKwon <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/19112
LGTM. Thanks! Merging to master and 2.2.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by tejasapatil <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by dongjoon-hyun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19112: [SPARK-21901][SS] Define toString for StateOperat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19112
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19112: [SPARK-21901][SS] Define toString for StateOperatorProgr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19112
**[Test build #81363 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81363/testReport)** for PR 19112 at commit [`337ad48`](https://github.com/apache/spark/commit/337ad489bb43ef93a651bdd4952bd7f0738698dc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org