[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA reported Intellij IDEA #17458

dbolshak · 2017-03-28T13:05:42Z

What changes were proposed in this pull request?

Few changes related to Intellij IDEA inspection.

How was this patch tested?

Changes were tested by existing unit tests

…eported Intellij IDEA

srowen

I brought forward some comments but everything else looks OK. I'd be OK accepting the other changes.

srowen · 2017-03-28T13:06:45Z

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

@@ -56,7 +56,7 @@ private[spark] class SparkUI private (
  with Logging
  with UIRoot {

-  val killEnabled = sc.map(_.conf.getBoolean("spark.ui.killEnabled", true)).getOrElse(false)
+  val killEnabled = sc.exists(_.conf.getBoolean("spark.ui.killEnabled", true))


Bringing forward some of my comments: I would probably not change this

srowen · 2017-03-28T13:07:34Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

-  def getTimeZoneOffset() : Int =
-    TimeZone.getDefault().getOffset(System.currentTimeMillis()) / 1000 / 60
+  def getTimeZoneOffset(): Int =
+    TimeZone.getDefault.getOffset(System.currentTimeMillis()) / 1000 / 60


I also wouldn't remove the parens here (though the convention was always to use parens when calling no-arg Java methods?) or elsewhere. That really isn't done consistently at all anyway.

srowen · 2017-03-28T13:09:01Z

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

@@ -31,7 +31,7 @@ private[ui] class JobsTab(parent: SparkUI) extends SparkUITab(parent, "jobs") {
  val operationGraphListener = parent.operationGraphListener

  def isFairScheduler: Boolean =
-    jobProgresslistener.schedulingMode == Some(SchedulingMode.FAIR)
+    jobProgresslistener.schedulingMode.contains(SchedulingMode.FAIR)


Likewise not sure about this change

srowen · 2017-03-28T13:09:20Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

@@ -290,7 +290,7 @@ private[ui] class StagePage(parent: StagesTab) extends WebUIPage("stage") {
        val _taskTable = new TaskPagedTable(
          parent.conf,
          UIUtils.prependBaseUri(parent.basePath) +
-            s"/stages/stage?id=${stageId}&attempt=${stageAttemptId}",
+            s"/stages/stage?id=$stageId&attempt=$stageAttemptId",


Agree they can be omitted but not sure it helps

the change was reverted.

jerryshao · 2017-03-28T13:22:27Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

@@ -317,7 +317,7 @@ private[spark] object UIUtils extends Logging {
    def getHeaderContent(header: String): Seq[Node] = {
      if (newlinesInHeader) {
        <ul class="unstyled">
-          { header.split("\n").map { case t => <li> {t} </li> } }
+          { header.split("\n").map(t => <li> {t} </li>)}


I think we normally use {} in this code, like map { t => xxx }.

That also works and gets rid of case. I personally prefer not opening a scope if it's not needed, and it isn't here, but it's minor as it's just a question of () vs {} and would rarely have any other impact.

I removed almost changes related to UI.

jerryshao · 2017-03-28T13:31:10Z

There're many changes related to UI part, actually we don't have many unit tests covered in this part, so I'm afraid these change may potentially introduce regression.

HyukjinKwon · 2017-03-28T14:13:42Z

core/src/main/scala/org/apache/spark/ui/jobs/StagesTab.scala

@@ -35,7 +35,7 @@ private[ui] class StagesTab(parent: SparkUI) extends SparkUITab(parent, "stages"
  attachPage(new StagePage(this))
  attachPage(new PoolPage(this))

-  def isFairScheduler: Boolean = progressListener.schedulingMode == Some(SchedulingMode.FAIR)
+  def isFairScheduler: Boolean = progressListener.schedulingMode.contains(SchedulingMode.FAIR)


Up to my knowledge, contains on Option breaks Scala 2.10 build (see #15393). I guess we are going to drop the support but not yet.

Thanks for that comment. I did not know that.

BTW, this was reverted.

It seems not reverted yet..

HyukjinKwon · 2017-03-28T14:15:52Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

-      val executorOverhead = (metrics.executorDeserializeTime +
-        metrics.resultSerializationTime)
+      val executorOverhead = metrics.executorDeserializeTime +
+        metrics.resultSerializationTime


I think we can make this inlined if more commits should be pushed.

HyukjinKwon · 2017-03-28T14:19:07Z

Probably, I guess this might be fine. Just in my experience, IntelliJ's inspection was quite okay except the case of breaking Scala 2.10. It might be better if they can be manually tested via UI though.

dbolshak

Folks, could you please go back to review? Looks like I reverted all my changes related to your comments.

HyukjinKwon · 2017-03-28T16:54:25Z

They are suggestions in my point of view. it doesn't necessarily mean you should follow if there are some reasons.

dbolshak · 2017-03-28T19:43:02Z

@HyukjinKwon could you please explain how to request to merge the PR?

srowen · 2017-03-28T19:54:07Z

core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala

@@ -34,9 +34,9 @@ private[ui] class AllStagesPage(parent: StagesTab) extends WebUIPage("") {
    listener.synchronized {
      val activeStages = listener.activeStages.values.toSeq
      val pendingStages = listener.pendingStages.values.toSeq
-      val completedStages = listener.completedStages.reverse.toSeq
+      val completedStages = listener.completedStages.reverse


I'm neutral on this because Seq.toSeq is a no-op anyway and, theoretically, the type of completedStages could change and then this isn't a Seq, but, in practice, wont' matter.

If it's really matter, it's better to add type to val completedStages, it describes your point much better then trying call no-op method. But it won't be consistent, so I would prefer to leave it as is.

For me not clear, do you request this change back or we can leave my changes.

srowen · 2017-03-28T19:54:37Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

@@ -103,7 +103,7 @@ private[ui] class StagePage(parent: StagesTab) extends WebUIPage("stage") {
      val taskSortColumn = Option(parameterTaskSortColumn).map { sortColumn =>
        UIUtils.decodeURLParameter(sortColumn)
      }.getOrElse("Index")
-      val taskSortDesc = Option(parameterTaskSortDesc).map(_.toBoolean).getOrElse(false)
+      val taskSortDesc = Option(parameterTaskSortDesc).exists(_.toBoolean)


Still not sure about whether this is clearer. I'm neutral. @HyukjinKwon is right about .contains I think so I'd double check this existed in 2.10?

There is no Option.contains call any more, it was in another place.

yes, I meant the question is whether .exists is in 2.10, but I am back at a desktop now and found it is, so no problem. That's not the issue, just, whether it's clearer

If my opinion can be helpful.. personally I prefer the previous one to explicitly show the default like the ones below. Actually, I intendedly use this pattern in several places.

Ok, I've reverted this back.

srowen · 2017-03-28T19:55:30Z

core/src/main/scala/org/apache/spark/ui/storage/RDDPage.scala

@@ -69,7 +69,7 @@ private[ui] class RDDPage(parent: StorageTab) extends WebUIPage("rdd") {
    }
    val blockTableHTML = try {
      val _blockTable = new BlockPagedTable(
-        UIUtils.prependBaseUri(parent.basePath) + s"/storage/rdd/?id=${rddId}",
+        UIUtils.prependBaseUri(parent.basePath) + s"/storage/rdd/?id=$rddId",


I still don't this it's worth unbracing. The braces aren't hard to read and maybe prevent some misinterpretation one day after a refactoring.

This one I've just reverted back.

…/apache/spark/ui/storage/RDDPage.scala

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

dbolshak · 2017-03-29T13:42:58Z

Looks like all comments have been addressed.

dbolshak · 2017-03-30T08:39:51Z

@srowen, @HyukjinKwon could you please merge the PR if it's ok of course?

HyukjinKwon · 2017-03-30T09:02:06Z

core/src/main/scala/org/apache/spark/ui/jobs/UIData.scala

-      newTaskInfo.setAccumulables(taskInfo.accumulables.filter {
-        accum => !accum.internal && accum.metadata != Some(AccumulatorContext.SQL_ACCUM_IDENTIFIER)
+      newTaskInfo.setAccumulables(taskInfo.accumulables.filter { acc =>
+        !acc.internal && !acc.metadata.contains(AccumulatorContext.SQL_ACCUM_IDENTIFIER)


We should revert this one too for the same reason in https://github.com/apache/spark/pull/17458/files#r108432518

HyukjinKwon · 2017-03-30T09:03:01Z

core/src/main/scala/org/apache/spark/ui/storage/RDDPage.scala

@@ -42,7 +42,7 @@ private[ui] class RDDPage(parent: StorageTab) extends WebUIPage("rdd") {

    val blockPage = Option(parameterBlockPage).map(_.toInt).getOrElse(1)
    val blockSortColumn = Option(parameterBlockSortColumn).getOrElse("Block Name")
-    val blockSortDesc = Option(parameterBlockSortDesc).map(_.toBoolean).getOrElse(false)
+    val blockSortDesc = Option(parameterBlockSortDesc).exists(_.toBoolean)


This also looks the same instance. Let's revert this change too.

srowen · 2017-03-30T09:31:41Z

core/src/main/scala/org/apache/spark/ui/WebUI.scala

@@ -116,15 +116,15 @@ private[spark] abstract class WebUI(
   * @param path Path in UI to unmount.
   */
  def removeStaticHandler(path: String): Unit = {
-    handlers.find(_.getContextPath() == path).foreach(detachHandler)
+    handlers.find(_.getContextPath == path).foreach(detachHandler)


@dbolshak this still isn't addressing comments that have been made several times. You can appreciate why I don't think it's worth the time to make changes like this given how much discussion it takes. Here's another example. Please read all previous comments and address all of them or close this.

dbolshak · 2017-03-30T10:25:22Z

Corrected.
Please take a look.
There is only one change (in 2 places) related to calling toSeq which has comment, but it's not clear can I leave my change or not.

HyukjinKwon · 2017-03-30T10:58:22Z

It looks good to me too.

SparkQA · 2017-03-30T13:24:51Z

Test build #3619 has finished for PR 17458 at commit 4788bbe.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2017-03-30T15:16:24Z

Merged to master

Denis Bolshakov added 4 commits March 28, 2017 14:59

[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA r…

8a31173

…eported Intellij IDEA

[SPARK-20127][CORE] Applying first comments from @srowen

6a7d2cc

[SPARK-20127][CORE] Second portion of applying comments

12cc39e

[SPARK-20127][CORE] inserting a space back to getTimeZoneOffset method

fa5a33c

srowen requested changes Mar 28, 2017

View reviewed changes

jerryshao reviewed Mar 28, 2017

View reviewed changes

[SPARK-20127][CORE] reverging some changes

751d9af

HyukjinKwon reviewed Mar 28, 2017

View reviewed changes

dbolshak commented Mar 28, 2017

View reviewed changes

srowen reviewed Mar 28, 2017

View reviewed changes

Denis Bolshakov added 3 commits March 28, 2017 23:26

[SPARK-20127][CORE] reverting back changes in core/src/main/scala/org…

ded9679

…/apache/spark/ui/storage/RDDPage.scala

[SPARK-20127][CORE] reverting back taskSortDesc

e606e41

[SPARK-20127][CORE] reverting back changes in

531e551

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

HyukjinKwon reviewed Mar 30, 2017

View reviewed changes

srowen reviewed Mar 30, 2017

View reviewed changes

Denis Bolshakov added 3 commits March 30, 2017 13:18

[SPARK-20127][CORE] revering few changes

882ca0c

[SPARK-20127][CORE] revering few changes

7016022

[SPARK-20127][CORE] revering few changes

4788bbe

srowen approved these changes Mar 30, 2017

View reviewed changes

asfgit closed this in 5e00a5d Mar 30, 2017

[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA reported Intellij IDEA #17458

[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA reported Intellij IDEA #17458

Conversation

dbolshak commented Mar 28, 2017

What changes were proposed in this pull request?

How was this patch tested?

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Mar 28, 2017 • edited Loading

dbolshak left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Mar 28, 2017

dbolshak commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolshak commented Mar 29, 2017

dbolshak commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolshak commented Mar 30, 2017

HyukjinKwon commented Mar 30, 2017

SparkQA commented Mar 30, 2017

srowen commented Mar 30, 2017

HyukjinKwon commented Mar 28, 2017 •

edited

Loading

HyukjinKwon Mar 28, 2017 •

edited

Loading