[SPARK-8695][CORE][MLlib] TreeAggregation shouldn't be triggered when it doesn't save wall-clock time.

[piganesh]

No description provided.

[srowen]

This needs a proper title and description.

[sarutak]

Doesn't this fix the issue mentioned in SPARK-8695?

[srowen]

Yeah, @mengxr was your intent to add the condition numPartitions > 5?
Also, I don't think you need to change the current condition; it's fine. (BTW .toDouble is a better idiom for casting than 1.0 *)

[sarutak]

@piganesh When you open a PR, you should put a proper title and description.
Also, you should add proper tags like [CORE] , [MLlib]. and so on.
In this case, the title should be like [SPARK-8695][CORE][MLlib] TreeAggregation shouldn't be triggered for 5 partitions.

See also https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark

[srowen]

(Ah: this continues a discussion in #7168 -- should have been mentioned in this PR.)

@piganesh do you mind closing this if you're not going to follow up?
Otherwise, please make the .toDouble change and correctly title this PR.

[piganesh]

Sean

I did generate a new patch based on mengxr 's comment

Namely

while (numPartitions > scale + math.ceil(1.0 * numPartitions / scale))

and submitted a pull request ..

thanks

• P. I.

[srowen]

@piganesh ... yes, that's this one. I've added a review comment last week, which you should act on.

[piganesh]

I had made the changes to use ceiling instead of the hard coded greater
than
5 check and submitted a pull request through ibmsoe github.

I need to check why you are not able to view the change. Give me a day.

thanks

From: Sean Owen [email protected]
To: apache/spark [email protected]
Cc: P I Ganesh/Austin/IBM@IBMUS
Date: 07/19/2015 04:58 PM
Subject: Re: [spark] Spark 8695 (#7397)

@piganesh ... yes, that's this one. I've added a review comment last week,
which you should act on.
—
Reply to this email directly or view it on GitHub.

[piganesh]

OK I understood now. You had a comment on top to use .toDouble. Shall get to it ASAP. Sorry for the confusion.

[piganesh]

Submitting a new pull request

[srowen]

LGTM except the parens around (numPartitions) aren't needed. That's trivial though.

[srowen]

OK to test

[SparkQA]

Test build #1148 has finished for PR 7397 at commit 041620c.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[sarutak]

retest this please.

[mengxr]

@piganesh You don't need to make a new PR for updates. You can push new commits to your remote branch, which you used to create the PR. Please address @srowen 's comment and remove (...) around numPartitions. Thanks!

[SparkQA]

Test build #37956 has finished for PR 7397 at commit 041620c.

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

[srowen]

Since this is taking a while to finish relative to the tiny change, I made this last change myself and merged to master.