[SPARK-7183][Network] Fix memory leak of TransportRequestHandler.streamIds

[viirya]

JIRA: https://issues.apache.org/jira/browse/SPARK-7183

[SparkQA]

Test build #31138 has finished for PR 5743 at commit 3b3f38a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[andrewor14]

@aarondav @rxin

[aarondav]

This is a reasonable solution, but I think that the actual issue that TransportRequestHandler has a list of streamIds at all. I think a different solution would be to have the StreamManager associated streams with channels (which is a documented guarantee already to StreamManager).

This would involve making StreamManager's getChunk take a TransportClient (or a "channelId", generated via channel.toString), which would update the StreamState, and then to make connectionTerminated also take this identifier (iterating over all streams to close associated ones).

[viirya]

@aarondav Sounds reasonable. However, this might increase more complexity to the codes. The current solution is very simple compared to the alternative one.

In addition to modifying TransportRequestHandler, StreamManager, OneForOneStreamManager, there are many existing unit tests involving StreamManager. Once StreamManager's current interface (especially getChunk) is updated, these tests are needed to largely modified. I think it is not worth doing that.

I update the codes based on your suggestion, but keep getChunk untouched. Please take a look. If you agree these additional complexity is not needed, I can revert it to current solution.

[SparkQA]

Test build #31213 has finished for PR 5743 at commit 37a4b6c.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[SparkQA]

Test build #31219 has finished for PR 5743 at commit 17f020f.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[viirya]

Looks like unrelated failure. Please test again.

[SparkQA]

Test build #31265 has finished for PR 5743 at commit 45908b7.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch adds the following new dependencies:
  • spark-unsafe_2.10-1.4.0-SNAPSHOT.jar

[aarondav]

Please remove this method, it should no longer be used.

[viirya]

@aarondav Thanks for comments. I updated the codes.

[SparkQA]

Test build #31547 has finished for PR 5743 at commit d35f19a.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class ExecutorUIData(

[SparkQA]

Test build #31549 has finished for PR 5743 at commit 97e205c.

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

[aarondav]

Ah, good point, this is safe because our Map is a ConcurrentHashMap (or else you would need to use an iterator to remove it safely). Would you mind making the left-hand type of the declaration of streams a ConcurrentHashMap? This is not the first place where we rely on the semantics of a ConcurrentHashMap over a general Map, and we should use proper style therefore.

[viirya]

Making sense. Updated.

[aarondav]

Just a couple remaining comments, otherwise this LGTM!

[SparkQA]

Test build #31574 has finished for PR 5743 at commit cf2c086.

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

[aarondav]

LGTM, merging in to master. Thanks!