[SPARK-4102] Remove unused ShuffleReader.stop() method.

[kayousterhout]

This method is not implemented by the only subclass
(HashShuffleReader), nor is it ever called. While the
use of Scala's fancy "???" was pretty exciting, the method's
existence can only lead to confusion and it therefore should
be deleted.

@mateiz was there a reason for adding this that I'm
missing?

[SparkQA]

Test build #22310 has started for PR 2966 at commit 904655e.

• This patch merges cleanly.

[SparkQA]

Test build #22310 has finished for PR 2966 at commit 904655e.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • abstract class Broadcast[T: ClassTag](val id: Long) extends Serializable with Logging

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22310/
Test PASSed.

[mateiz]

IMO we should keep this because it will be needed for other shuffle managers that launch threads. Because this is a developer API (where other people may plug in their own shuffle managers), we can't add it back later without breaking their code.

[mateiz]

BTW the ??? should indeed be changed to {}, it's weird that it says not implemented.

[kayousterhout]

But if other shuffle managers implement this, it won't ever be called,
which seems confusing/misleading. Do you mean we should add a call to
ShuffleReader.stop() somewhere?

On Mon, Oct 27, 2014 at 5:02 PM, Matei Zaharia notifications@github.com
wrote:

BTW the ??? should indeed be changed to {}, it's weird that it says not
implemented.

—
Reply to this email directly or view it on GitHub
#2966 (comment).

[kayousterhout]

As discussed offline with @mateiz, added back a commented-out version of the stop() method for use if/when this someday becomes a developer API

[SparkQA]

Test build #22477 has started for PR 2966 at commit 532c564.

• This patch merges cleanly.

[SparkQA]

Test build #22477 has finished for PR 2966 at commit 532c564.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22477/
Test PASSed.

[mateiz]

LGTM