Skip to content

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Jul 23, 2017

What changes were proposed in this pull request?

This PR avoids to reuse unpersistent dataset among test cases by making dataset unpersistent at the end of each test case.

In DatasetCacheSuite, the test case "get storage level" does not make dataset unpersisit after make the dataset persisitent. The same dataset will be made persistent by the test case "persist and then rebind right encoder when join 2 datasets" Thus, we run these test cases, the second case does not perform to make dataset persistent. This is because in

When we run only the second case, it performs to make dataset persistent. It is not good to change behavior of the second test suite. The first test case should correctly make dataset unpersistent.

Testing started at 17:52 ...
01:52:15.053 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
01:52:48.595 WARN org.apache.spark.sql.execution.CacheManager: Asked to cache already cached data.
01:52:48.692 WARN org.apache.spark.sql.execution.CacheManager: Asked to cache already cached data.
01:52:50.864 WARN org.apache.spark.storage.RandomBlockReplicationPolicy: Expecting 1 replicas with only 0 peer/s.
01:52:50.864 WARN org.apache.spark.storage.RandomBlockReplicationPolicy: Expecting 1 replicas with only 0 peer/s.
01:52:50.868 WARN org.apache.spark.storage.BlockManager: Block rdd_8_1 replicated to only 0 peer(s) instead of 1 peers
01:52:50.868 WARN org.apache.spark.storage.BlockManager: Block rdd_8_0 replicated to only 0 peer(s) instead of 1 peers

After this PR, these messages do not appear

Testing started at 18:14 ...
02:15:05.329 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable

Process finished with exit code 0

How was this patch tested?

Used the existing test

@kiszk
Copy link
Member Author

kiszk commented Jul 23, 2017

@gatorsmile could you please review this?

@SparkQA
Copy link

SparkQA commented Jul 23, 2017

Test build #79883 has finished for PR 18719 at commit 3e4438d.

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

@kiszk
Copy link
Member Author

kiszk commented Jul 23, 2017

cc @cloud-fan

@cloud-fan
Copy link
Contributor

shall we add a afterEach to clean up the CacheManager?

@kiszk
Copy link
Member Author

kiszk commented Jul 23, 2017

Good suggestion, make sense. done.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SparkQA
Copy link

SparkQA commented Jul 23, 2017

Test build #79889 has finished for PR 18719 at commit e9d9a33.

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

@gatorsmile
Copy link
Member

Thanks! Merging to master.

@asfgit asfgit closed this in 481f079 Jul 23, 2017

// Clear all persistent datasets after each test
override def afterEach(): Unit = {
spark.sharedState.cacheManager.clearCache()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    try {
      // Clear all persistent datasets after each test
      spark.sharedState.cacheManager.clearCache()
    } finally {
      super.afterEach()
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, ideally we should always call super.afterEach, @kiszk can you send a follow-up PR? thx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted #18721

@kiszk
Copy link
Member Author

kiszk commented Jul 24, 2017

good catch, i will create a follow-up PR today.

ghost pushed a commit to dbtsai/spark that referenced this pull request Jul 25, 2017
…must call super.afterEach()

## What changes were proposed in this pull request?

This PR ensures to call `super.afterEach()` in overriding `afterEach()` method in `DatasetCacheSuite`. When we override `afterEach()` method in Testsuite, we have to call `super.afterEach()`.

This is a follow-up of apache#18719 and SPARK-21512.

## How was this patch tested?

Used the existing test suite

Author: Kazuaki Ishizaki <[email protected]>

Closes apache#18721 from kiszk/SPARK-21516.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants