Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jaceklaskowski wants to merge 3 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
337ad48
[SPARK-21901][SS] Define toString for StateOperatorProgress
jaceklaskowski Sep 3, 2017
0e2b9c6
After review: reverting unrelated/stylistic changes
jaceklaskowski Sep 5, 2017
673e088
Merge remote-tracking branch 'origin/master' into SPARK-21901-StateOp…
jaceklaskowski Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions sql/core/src/main/scala/org/apache/spark/sql/streaming/progress.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ class StateOperatorProgress private[sql](
("numRowsUpdated" -> JInt(numRowsUpdated)) ~
("memoryUsedBytes" -> JInt(memoryUsedBytes))
}

override def toString: String = prettyJson
}

/**
Expand Down Expand Up @@ -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))
Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jaceklaskowski jaceklaskowski Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

("startOffset" -> tryParse(startOffset)) ~
("endOffset" -> tryParse(endOffset)) ~
("numInputRows" -> JInt(numInputRows)) ~
("inputRowsPerSecond" -> safeDoubleToJValue(inputRowsPerSecond)) ~
("processedRowsPerSecond" -> safeDoubleToJValue(processedRowsPerSecond))
}

private def tryParse(json: String) = try {
Expand All @@ -200,7 +202,7 @@ class SourceProgress protected[sql](
*/
@InterfaceStability.Evolving
class SinkProgress protected[sql](
val description: String) extends Serializable {
Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jaceklaskowski jaceklaskowski Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

@jaceklaskowski jaceklaskowski Sep 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I fix the other places then (to make the code consistent and according to the rules)?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@tejasapatil tejasapatil Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@HyukjinKwon HyukjinKwon Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val description: String) extends Serializable {

/** The compact JSON representation of this progress. */
def json: String = compact(render(jsonValue))
Expand All @@ -211,6 +213,6 @@ class SinkProgress protected[sql](
override def toString: String = prettyJson

private[sql] def jsonValue: JValue = {
("description" -> JString(description))
"description" -> JString(description)
}
}