You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by dbolshak <gi...@git.apache.org> on 2017/03/28 13:05:43 UTC
[GitHub] spark pull request #17458: [SPARK-20127][CORE] few warning have been fixed w...
GitHub user dbolshak opened a pull request:
https://github.com/apache/spark/pull/17458
[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA reported Intellij IDEA
## 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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dbolshak/spark SPARK-20127
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17458.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 #17458
----
commit 8a31173b436eb7d39f2423eccda7932239f8d5b6
Author: Denis Bolshakov <de...@onefactor.com>
Date: 2017-03-28T11:59:01Z
[SPARK-20127][CORE] few warning have been fixed which Intellij IDEA reported Intellij IDEA
commit 6a7d2ccc1c529ddca1765640073ae4a5316e51f7
Author: Denis Bolshakov <de...@onefactor.com>
Date: 2017-03-28T12:30:16Z
[SPARK-20127][CORE] Applying first comments from @srowen
commit 12cc39ee3974e6412fa9bdfb8915508a02b75e57
Author: Denis Bolshakov <de...@onefactor.com>
Date: 2017-03-28T12:46:27Z
[SPARK-20127][CORE] Second portion of applying comments
----
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17458
They are suggestions in my point of view. it doesn't necessarily mean you should follow if there are some reasons.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17458
**[Test build #3619 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3619/testReport)** for PR 17458 at commit [`4788bbe`](https://github.com/apache/spark/commit/4788bbef279828d96feead227b4b450e96493d4c).
* 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
[GitHub] spark pull request #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108528285
--- Diff: 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)
--- End diff --
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 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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108414748
--- Diff: 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",
--- End diff --
Agree they can be omitted but not sure it helps
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17458
Merged to master
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on the issue:
https://github.com/apache/spark/pull/17458
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108874623
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/UIData.scala ---
@@ -180,8 +180,8 @@ private[spark] object UIData {
speculative = taskInfo.speculative
)
newTaskInfo.gettingResultTime = taskInfo.gettingResultTime
- 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)
--- End diff --
We should revert this one too for the same reason in https://github.com/apache/spark/pull/17458/files#r108432518
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108414149
--- Diff: 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))
--- End diff --
Bringing forward some of my comments: I would probably not change this
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108433133
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala ---
@@ -786,8 +786,8 @@ private[ui] object StagePage {
info: TaskInfo, metrics: TaskMetricsUIData, currentTime: Long): Long = {
if (info.finished) {
val totalExecutionTime = info.finishTime - info.launchTime
- val executorOverhead = (metrics.executorDeserializeTime +
- metrics.resultSerializationTime)
+ val executorOverhead = metrics.executorDeserializeTime +
+ metrics.resultSerializationTime
--- End diff --
I think we can make this inlined if more commits should be pushed.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108414675
--- Diff: 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)
--- End diff --
Likewise not sure about this change
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108432518
--- Diff: 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)
--- End diff --
Up to my knowledge, `contains` on `Option` breaks Scala 2.10 build (see https://github.com/apache/spark/pull/15393). I guess we are going to drop the support but not yet.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108874489
--- Diff: 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)
--- End diff --
It seems not reverted yet..
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108527691
--- Diff: 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)
--- End diff --
There is no `Option.contains` call any more, it was in another place.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17458
Can one of the admins verify this patch?
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108880632
--- Diff: 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)
--- End diff --
@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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108521234
--- Diff: 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)
--- End diff --
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?
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108677907
--- Diff: 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)
--- End diff --
BTW, this was reverted.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17458
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108529238
--- Diff: 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)
--- End diff --
Thanks for that comment. I did not know that.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108414360
--- Diff: core/src/main/scala/org/apache/spark/ui/UIUtils.scala ---
@@ -513,8 +513,8 @@ private[spark] object UIUtils extends Logging {
param
}
- def getTimeZoneOffset() : Int =
- TimeZone.getDefault().getOffset(System.currentTimeMillis()) / 1000 / 60
+ def getTimeZoneOffset(): Int =
+ TimeZone.getDefault.getOffset(System.currentTimeMillis()) / 1000 / 60
--- End diff --
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17458
**[Test build #3619 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3619/testReport)** for PR 17458 at commit [`4788bbe`](https://github.com/apache/spark/commit/4788bbef279828d96feead227b4b450e96493d4c).
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108678514
--- Diff: 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
--- End diff --
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17458
Probably, I guess this should 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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on the issue:
https://github.com/apache/spark/pull/17458
Looks like all comments have been addressed.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108521452
--- Diff: 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",
--- End diff --
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108418582
--- Diff: 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>)}
--- End diff --
I think we normally use `{}` in this code, like `map { t => xxx }`.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on the issue:
https://github.com/apache/spark/pull/17458
@HyukjinKwon could you please explain how to request to merge the PR?
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on the issue:
https://github.com/apache/spark/pull/17458
@srowen, @HyukjinKwon could you please merge the PR if it's ok of course?
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108677770
--- Diff: 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",
--- End diff --
the change was reverted.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the issue:
https://github.com/apache/spark/pull/17458
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108890304
--- Diff: 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
--- End diff --
For me not clear, do you request this change back or we can leave my changes.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108521117
--- Diff: 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
--- End diff --
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 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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108421849
--- Diff: 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>)}
--- End diff --
I removed almost changes related to UI.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed which In...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17458
It looks good to me too.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108528243
--- Diff: 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",
--- End diff --
This one I've just reverted back.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108420602
--- Diff: 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>)}
--- End diff --
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.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by dbolshak <gi...@git.apache.org>.
Github user dbolshak commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108589069
--- Diff: 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)
--- End diff --
Ok, I've reverted this back.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108560956
--- Diff: 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)
--- End diff --
If my opinion can be helpful, personally I prefer the previous one to explicitly show the default. Actually, I intendedly use this pattern in several places.
---
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 #17458: [SPARK-20127][CORE] few warning have been fixed w...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17458#discussion_r108874836
--- Diff: 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)
--- End diff --
This also looks the same instance. Let's revert this change too.
---
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