[SPARK-21901][SS] Define toString for StateOperatorProgress #19112
Conversation
| @@ -200,7 +202,7 @@ class SourceProgress protected[sql]( | |||
| */ | |||
| @InterfaceStability.Evolving | |||
| class SinkProgress protected[sql]( | |||
| val description: String) extends Serializable { | |||
There was a problem hiding this comment.
Hi, @jaceklaskowski . For this, the original one looks correct for me.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes. Please refer here, https://github.com/databricks/scala-style-guide
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
}There was a problem hiding this comment.
Should I fix the other places then (to make the code consistent and according to the rules)?
There was a problem hiding this comment.
Sorry, I'm not in a position to say that. Maybe, committers will review this and give a direction.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am a committer. Generally, a PR should targets an issue without other changes. I use common sense - fixing styles around the changes (rather small scope) is fine if it does not look making revert/backport harder.
| ("endOffset" -> tryParse(endOffset)) ~ | ||
| ("numInputRows" -> JInt(numInputRows)) ~ | ||
| ("inputRowsPerSecond" -> safeDoubleToJValue(inputRowsPerSecond)) ~ | ||
| ("processedRowsPerSecond" -> safeDoubleToJValue(processedRowsPerSecond)) |
There was a problem hiding this comment.
nit, the original style seems to be more frequent and looks correct, doesn't it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep. I guess that those are the less frequent style.
|
Test build #81363 has finished for PR 19112 at commit
|
|
Test build #81406 has finished for PR 19112 at commit
|
|
Test build #81407 has finished for PR 19112 at commit
|
|
Hey @HyukjinKwon, as the only committer who's been involved in this PR, could you review it again and possibly merge to master? Thanks. |
|
LGTM. Thanks! Merging to master and 2.2. |
## 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. Author: Jacek Laskowski <jacek@japila.pl> Closes #19112 from jaceklaskowski/SPARK-21901-StateOperatorProgress-toString. (cherry picked from commit fa0092b) Signed-off-by: Shixiong Zhu <zsxwing@gmail.com>
jaceklaskowski
deleted the
SPARK-21901-StateOperatorProgress-toString
branch
## 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. Author: Jacek Laskowski <jacek@japila.pl> Closes apache#19112 from jaceklaskowski/SPARK-21901-StateOperatorProgress-toString. (cherry picked from commit fa0092b) Signed-off-by: Shixiong Zhu <zsxwing@gmail.com>
What changes were proposed in this pull request?
Just
StateOperatorProgress.toString+ few formatting fixesHow was this patch tested?
Local build. Waiting for OK from Jenkins.