[SPARK-4467] fix elements read count for ExtrenalSorter #3302

tsdeng · 2014-11-17T06:41:18Z

the elementsRead variable should be reset to 0 after each spilling

AmplabJenkins · 2014-11-17T06:42:09Z

Can one of the admins verify this patch?

sryza · 2014-11-17T08:43:24Z

As far as I can tell, elementsRead isn't used for anything. Would we be able to just remove it entirely?

sryza · 2014-11-17T08:44:00Z

Also, mind filing a JIRA for this, or, if one already exists, including the name in the title here?

tsdeng · 2014-11-17T17:53:30Z

I think it's used for controlling the frequency of checking to spilling. It's used in Spillable.scala.
Without this fix, it may spill very small files.

tsdeng · 2014-11-17T18:08:09Z

I guess, the original intention of this variable is to make sure there is at least 1K records before each spilling. We saw a too many files open exception due to this variable is not being updated correctly. Of course , this is not the root cause of the issue, I currently have another working branch trying to tackle the deeper cause of this, as mentioned in https://issues.apache.org/jira/browse/SPARK-4452. But at the same time, I'm sending this PR to fix the updating the elementsRead to alleviate the problem

sryza · 2014-11-17T18:56:57Z

That makes sense, my IDE for some reason didn't show me the usage in Spillable.scala. In that case, this change makes sense.

Spilling is also based on the amount of memory taken up. Do you know what the thinking is for basing it on the number of records as well?

tsdeng · 2014-11-17T19:05:46Z

From my understanding, this variable is used as a lower bound of the number of records when spilling. It's useful when the memory is really low.

sryza · 2014-11-17T19:37:58Z

I don't entirely understand that line of argument. Why would we want to place a lower bound if the data structure is pushing the memory threshold? I filed https://issues.apache.org/jira/browse/SPARK-4456 to figure this out and document it better.

This patch LGTM though.

tsdeng · 2014-11-17T19:42:38Z

Yeah, I guess we need that lower bound when memory threshold is not "pushable", meaning when memory is too small and you can not acquire memory... I agree this behavior is kinda weird. Maybe it's used as "the last defense"? Please refer to https://issues.apache.org/jira/browse/SPARK-4452 why this last defense saved my svd job. I'm also working on another branch documented on SPARK-4452 to make a deeper fix for memory allocation for spillable objects

andrewor14 · 2014-11-17T23:40:17Z

add to whitelist

andrewor14 · 2014-11-17T23:43:32Z

I see, after a while we unconditionally try to spill every 32 elements regardless of whether the in-memory buffer has exceeded the spill threshold. This is a serious problem and it seems that this is just an omission in the original code since we don't ever update elementsRead ever in this code path. Changes here LGTM.

I think this is the first step towards fixing the too many files open issue that many are seeing. We still need to hunt down the root cause for why the lower bound for how much memory a data structure can have is not being accounted for properly.

SparkQA · 2014-11-17T23:45:14Z

Test build #23515 has started for PR 3302 at commit 74ca246.

This patch merges cleanly.

mateiz · 2014-11-17T23:46:54Z

@tsdeng this patch looks good but wouldn't it be better to set it to 0 in Spillable, after it calls spill()? Then it will fix this problem in all subclasses, and you can remove the code that sets it to 0 in ExternalAppendOnlyMap.

sryza · 2014-11-17T23:56:11Z

after a while we unconditionally try to spill every 32 elements regardless of whether the in-memory buffer has exceeded the spill threshold.

The code still verifies currentMemory >= myMemoryThreshold as well, right?

mateiz · 2014-11-18T00:00:49Z

@sryza yeah it does, the problem is just that myMemoryThreshold turns to 0 after you spill. The idea was to wait for at least 1000 more elements before requesting memory, but it currently doesn't, and it gets a 0 returned.

SparkQA · 2014-11-18T01:10:41Z

Test build #23515 has finished for PR 3302 at commit 74ca246.

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

AmplabJenkins · 2014-11-18T01:10:44Z

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

tsdeng · 2014-11-18T01:18:25Z

@mateiz
agree, but elementsRead is a def in Spillable, does adding a resetElementsRead sounds good to you?

mateiz · 2014-11-18T01:21:59Z

Oh, weird, it is. What about changing it to a var, any problems with that?

mateiz · 2014-11-18T01:23:08Z

Basically it is weird to have a var shared with subclasses, but I think this will be more obvious. Probably the very best way is to make elementsRead be modified only in Spillable, and add a method called addElementRead or something like that subclasses can call to tell it to increment the count.

tsdeng · 2014-11-18T02:05:22Z

Ha, my bad, elementsRead is a var, but also like to keep it private or protected and make addElementRead a method to manipulate it

mateiz · 2014-11-18T02:09:18Z

Sounds good.

SparkQA · 2014-11-18T03:07:45Z

Test build #23533 has started for PR 3302 at commit bb7ff28.

This patch merges cleanly.

This is the 1.1 version of #3302. There has been some refactoring in master so we can't cherry-pick that PR. Author: Andrew Or <andrew@databricks.com> Closes #3330 from andrewor14/sort-fetch-fail and squashes the following commits: 486fc49 [Andrew Or] Reset `elementsRead`

SparkQA · 2014-11-18T04:31:23Z

Test build #23533 has finished for PR 3302 at commit bb7ff28.

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

AmplabJenkins · 2014-11-18T04:31:27Z

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

mateiz · 2014-11-18T05:57:22Z

core/src/main/scala/org/apache/spark/util/collection/Spillable.scala

+
+  // subclass calls this method to notify reading an element
+  // it's used to check spilling frequency
+  protected def addElementsRead = elementsRead += 1


Add parentheses to this and call it with addElementsRead() since it has side effects.

SparkQA · 2014-11-18T18:50:25Z

Test build #23553 has started for PR 3302 at commit 782c7de.

This patch merges cleanly.

SparkQA · 2014-11-18T20:15:50Z

Test build #23553 has finished for PR 3302 at commit 782c7de.

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

AmplabJenkins · 2014-11-18T20:15:54Z

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

andrewor14 · 2014-11-18T21:13:28Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

@@ -132,7 +130,7 @@ class ExternalAppendOnlyMap[K, V, C](
        currentMap = new SizeTrackingAppendOnlyMap[K, C]
      }
      currentMap.changeValue(curEntry._1, update)
-      elementsRead += 1
+      addElementsRead


you still need the () here and everywhere this is used

andrewor14 · 2014-11-18T21:14:46Z

A few more minor comments. This LGTM otherwise

SparkQA · 2014-11-18T22:00:40Z

Test build #23564 has started for PR 3302 at commit 7b56ca0.

This patch merges cleanly.

SparkQA · 2014-11-18T23:23:50Z

Test build #23564 has finished for PR 3302 at commit 7b56ca0.

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

AmplabJenkins · 2014-11-18T23:23:54Z

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

andrewor14 · 2014-11-18T23:31:11Z

@mateiz any other comments?

mateiz · 2014-11-19T07:27:07Z

@andrewor14 nope, it looks good.

andrewor14 · 2014-11-19T18:00:03Z

Ok, merging into master and 1.2. Thanks @tsdeng.
The 1.1 version was previously merged at #3330.

the elementsRead variable should be reset to 0 after each spilling Author: Tianshuo Deng <tdeng@twitter.com> Closes #3302 from tsdeng/fix_external_sorter_record_count and squashes the following commits: 7b56ca0 [Tianshuo Deng] fix method signature 782c7de [Tianshuo Deng] make elementsRead private, fix comment bb7ff28 [Tianshuo Deng] update elemetsRead through addElementsRead method 74ca246 [Tianshuo Deng] fix elements read count (cherry picked from commit d75579d) Signed-off-by: Andrew Or <andrew@databricks.com>

fix elements read count

74ca246

tsdeng changed the title ~~fix elements read count for ExtrenalSorter~~ [SPARK-4452] fix elements read count for ExtrenalSorter Nov 17, 2014

andrewor14 mentioned this pull request Nov 18, 2014

[SPARK-4467] Partial fix for fetch failure in sort-based shuffle (1.1) #3330

Closed

update elemetsRead through addElementsRead method

bb7ff28

mateiz reviewed Nov 18, 2014
View reviewed changes

tsdeng changed the title ~~[SPARK-4452] fix elements read count for ExtrenalSorter~~ [SPARK-4467] fix elements read count for ExtrenalSorter Nov 18, 2014

make elementsRead private, fix comment

782c7de

andrewor14 reviewed Nov 18, 2014
View reviewed changes

fix method signature

7b56ca0

asfgit closed this in d75579d Nov 19, 2014

[SPARK-4467] fix elements read count for ExtrenalSorter #3302

[SPARK-4467] fix elements read count for ExtrenalSorter #3302

Conversation

tsdeng commented Nov 17, 2014

AmplabJenkins commented Nov 17, 2014

sryza commented Nov 17, 2014

sryza commented Nov 17, 2014

tsdeng commented Nov 17, 2014

tsdeng commented Nov 17, 2014

sryza commented Nov 17, 2014

tsdeng commented Nov 17, 2014

sryza commented Nov 17, 2014

tsdeng commented Nov 17, 2014

andrewor14 commented Nov 17, 2014

andrewor14 commented Nov 17, 2014

SparkQA commented Nov 17, 2014

mateiz commented Nov 17, 2014

sryza commented Nov 17, 2014

mateiz commented Nov 18, 2014

SparkQA commented Nov 18, 2014

AmplabJenkins commented Nov 18, 2014

tsdeng commented Nov 18, 2014

mateiz commented Nov 18, 2014

mateiz commented Nov 18, 2014

tsdeng commented Nov 18, 2014

mateiz commented Nov 18, 2014

SparkQA commented Nov 18, 2014

SparkQA commented Nov 18, 2014

AmplabJenkins commented Nov 18, 2014

mateiz Nov 18, 2014

Choose a reason for hiding this comment

SparkQA commented Nov 18, 2014

SparkQA commented Nov 18, 2014

AmplabJenkins commented Nov 18, 2014

andrewor14 Nov 18, 2014

Choose a reason for hiding this comment

andrewor14 commented Nov 18, 2014

SparkQA commented Nov 18, 2014

SparkQA commented Nov 18, 2014

AmplabJenkins commented Nov 18, 2014

andrewor14 commented Nov 18, 2014

mateiz commented Nov 19, 2014

andrewor14 commented Nov 19, 2014