You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by original-brownbear <gi...@git.apache.org> on 2017/09/16 14:54:39 UTC
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
GitHub user original-brownbear opened a pull request:
https://github.com/apache/spark/pull/19254
[MINOR][CORE] Cleanup dead code and duplication in Mem. Management
## What changes were proposed in this pull request?
* Removed the method `org.apache.spark.sql.catalyst.expressions.codegen.UnsafeRowWriter#alignToWords`.
It became unused as a result of 85b0a157543201895557d66306b38b3ca52f2151
(SPARK-15962) introducing word alignment for unsafe arrays.
* Cleaned up duplicate code in memory management and unsafe sorters
* The change extracting the exception paths is more than just cosmetics since it def. reduces the size the affected methods compile to
## How was this patch tested?
* Build still passes after removing the method, grepping the codebase for `alignToWords` shows no reference to it anywhere either.
* Dried up code is covered by existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/original-brownbear/spark cleanup-mem-consumer
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19254.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 #19254
----
commit 84094a0c75727158f96a942d8d80cc3f5721b955
Author: Armin <me...@obrown.io>
Date: 2017-09-16T13:34:23Z
[MINOR][CORE] Cleanup dead code and duplication in mem. management
* Removed the method `org.apache.spark.sql.catalyst.expressions.codegen.UnsafeRowWriter#alignToWords`.
It became unused as a result of 85b0a157543201895557d66306b38b3ca52f2151
(SPARK-15962) introducing word alignment for unsafe arrays.
* Cleaned up duplicate code in memory management and unsafe sorters
* The change extracting the exception paths is more than just cosmetics since it def. reduces the size the affected methods compile to
* Build still passes after removing the method, grepping the codebase for `alignToWords` shows no reference to it anywhere either.
* Dried up code is covered by existing tests.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3924 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3924/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on a diff in the pull request:
https://github.com/apache/spark/pull/19254#discussion_r139305653
--- Diff: core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java ---
@@ -403,6 +391,16 @@ public long spill(long numBytes) throws IOException {
public void remove() {
throw new UnsupportedOperationException();
}
+
+ private void handleFailedDelete() {
+ // remove the spill file from disk
+ File file = spillWriters.removeFirst().getFile();
+ if (file != null && file.exists()) {
+ if (!file.delete()) {
--- End diff --
Right, will collapse those :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3924 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3924/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3922 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3922/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19254
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3925 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3925/testReport)** for PR 19254 at commit [`6b408a0`](https://github.com/apache/spark/commit/6b408a0314c4cf9fbd371712175c95942aae2a89).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on the issue:
https://github.com/apache/spark/pull/19254
Seems to be broken on Jenkins `master` too https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test/job/spark-master-test-sbt-hadoop-2.7/3490/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19254#discussion_r139304891
--- Diff: core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java ---
@@ -403,6 +391,16 @@ public long spill(long numBytes) throws IOException {
public void remove() {
throw new UnsupportedOperationException();
}
+
+ private void handleFailedDelete() {
+ // remove the spill file from disk
+ File file = spillWriters.removeFirst().getFile();
+ if (file != null && file.exists()) {
+ if (!file.delete()) {
--- End diff --
The two if conditions could be collapsed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19254
I think it's a problem with a recent change, yeah. Just wait until it can be resolved then we can retest
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3923 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3923/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3925 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3925/testReport)** for PR 19254 at commit [`6b408a0`](https://github.com/apache/spark/commit/6b408a0314c4cf9fbd371712175c95942aae2a89).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3922 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3922/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on a diff in the pull request:
https://github.com/apache/spark/pull/19254#discussion_r139307959
--- Diff: core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java ---
@@ -403,6 +391,16 @@ public long spill(long numBytes) throws IOException {
public void remove() {
throw new UnsupportedOperationException();
}
+
+ private void handleFailedDelete() {
+ // remove the spill file from disk
+ File file = spillWriters.removeFirst().getFile();
+ if (file != null && file.exists()) {
+ if (!file.delete()) {
--- End diff --
fixed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on the issue:
https://github.com/apache/spark/pull/19254
Test failure looks unrelated (even though it happened twice in a row now):
```sh
[info] org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite *** ABORTED *** (1 second, 114 milliseconds)
[info] spark-submit returned with exit code 1.
[info] Command line: './bin/spark-submit' '--name' 'prepare testing tables' '--master' 'local[2]' '--conf' 'spark.ui.enabled=false' '--conf' 'spark.master.rest.enabled=false' '--conf' 'spark.sql.warehouse.dir=/home/jenkins/workspace/NewSparkPullRequestBuilder/target/tmp/warehouse-10257aaf-1819-4ac2-a504-d4d325c474eb' '--conf' 'spark.sql.test.version.index=0' '--driver-java-options' '-Dderby.system.home=/home/jenkins/workspace/NewSparkPullRequestBuilder/target/tmp/warehouse-10257aaf-1819-4ac2-a504-d4d325c474eb' '/home/jenkins/workspace/NewSparkPullRequestBuilder/target/tmp/test5614170310518985767.py'
[info]
[info] 2017-09-17 06:11:01.557 - stderr> Error: Could not find or load main class org.apache.spark.launcher.Main (SparkSubmitTestUtils.scala:81)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions$class.newAssertionFailedException(Assertions.scala:528)
[info] at org.scalatest.FunSuite.newAssertionFailedException(FunSuite.scala:1560)
[info] at org.scalatest.Assertions$class.fail(Assertions.scala:1089)
[info] at org.scalatest.FunSuite.fail(FunSuite.scala:1560)
[info] at org.apache.spark.sql.hive.SparkSubmitTestUtils$class.runSparkSubmit(SparkSubmitTestUtils.scala:81)
[info] at org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite.runSparkSubmit(HiveExternalCatalogVersionsSuite.scala:38)
[info] at org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite$$anonfun$beforeAll$1.apply(HiveExternalCatalogVersionsSuite.scala:120)
[info] at org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite$$anonfun$beforeAll$1.apply(HiveExternalCatalogVersionsSuite.scala:105)
[info] at scala.collection.immutable.List.foreach(List.scala:381)
[info] at org.apache.spark.sql.hive.HiveExternalCatalogVersionsSuite.beforeAll(HiveExternalCatalogVersionsSuite.scala:105)
[info] at org.scalatest.BeforeAndAfterAll$class.liftedTree1$1(BeforeAndAfterAll.scala:212)
[info] at org.scalatest.BeforeAndAfterAll$class.run(BeforeAndAfterAll.scala:210)
[info] at org.apache.spark.SparkFunSuite.run(SparkFunSuite.scala:31)
[info] at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:314)
[info] at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:480)
[info] at sbt.ForkMain$Run$2.call(ForkMain.java:296)
[info] at sbt.ForkMain$Run$2.call(ForkMain.java:286)
[info] at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
[info] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
[info] at java.lang.Thread.run(Thread.java:745)
```
The same test passes fine for me locally.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19254
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19254
**[Test build #3923 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3923/testReport)** for PR 19254 at commit [`84094a0`](https://github.com/apache/spark/commit/84094a0c75727158f96a942d8d80cc3f5721b955).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on the issue:
https://github.com/apache/spark/pull/19254
Test failure in PySpark appears unrelated, various tests OOMed like e.g.
```sh
FAILED (errors=2)
[Running <class 'pyspark.streaming.tests.StreamingListenerTests'>]
ERROR
======================================================================
ERROR: setUpClass (pyspark.streaming.tests.StreamingListenerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/pyspark/streaming/tests.py", line 65, in setUpClass
cls.sc = SparkContext(appName=class_name, conf=conf)
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/pyspark/context.py", line 118, in __init__
conf, jsc, profiler_cls)
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/pyspark/context.py", line 180, in _do_init
self._jsc = jsc or self._initialize_context(self._conf._jconf)
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/pyspark/context.py", line 270, in _initialize_context
return self._jvm.JavaSparkContext(jconf)
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/lib/py4j-0.10.6-src.zip/py4j/java_gateway.py", line 1428, in __call__
answer, self._gateway_client, None, self._fqn)
File "/home/jenkins/workspace/NewSparkPullRequestBuilder/python/lib/py4j-0.10.6-src.zip/py4j/protocol.py", line 320, in get_return_value
format(target_id, ".", name), value)
py4j.protocol.Py4JJavaError: An error occurred while calling None.org.apache.spark.api.java.JavaSparkContext.
: java.lang.OutOfMemoryError: Java heap space
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on a diff in the pull request:
https://github.com/apache/spark/pull/19254#discussion_r139305647
--- Diff: core/src/main/java/org/apache/spark/memory/MemoryConsumer.java ---
@@ -116,13 +110,7 @@ public void freeArray(LongArray array) {
protected MemoryBlock allocatePage(long required) {
MemoryBlock page = taskMemoryManager.allocatePage(Math.max(pageSize, required), this);
if (page == null || page.size() < required) {
--- End diff --
@srowen I didn't do this on purpose so that `allocatePage` compiles smaller since the condition isn't true very often. Not worth the duplication here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on the issue:
https://github.com/apache/spark/pull/19254
Sure, added a JIRA for the failure here https://issues.apache.org/jira/browse/SPARK-22047 :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by original-brownbear <gi...@git.apache.org>.
Github user original-brownbear commented on the issue:
https://github.com/apache/spark/pull/19254
@srowen rebased against `master` to get the test ignore https://github.com/apache/spark/commit/894a7561de2c2ff01fe7fcc5268378161e9e5643 , should be good to retest now :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19254: [MINOR][CORE] Cleanup dead code and duplication in Mem. ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19254
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19254: [MINOR][CORE] Cleanup dead code and duplication i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19254#discussion_r139304877
--- Diff: core/src/main/java/org/apache/spark/memory/MemoryConsumer.java ---
@@ -116,13 +110,7 @@ public void freeArray(LongArray array) {
protected MemoryBlock allocatePage(long required) {
MemoryBlock page = taskMemoryManager.allocatePage(Math.max(pageSize, required), this);
if (page == null || page.size() < required) {
--- End diff --
While here, pull this check into the method and call it "checkAllocation" or something?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org