[SPARK-31608][CORE][WEBUI][TEST] Add test suites for HybridStore and HistoryServerMemoryManager #29509

baohe-zhang · 2020-08-21T16:06:38Z

What changes were proposed in this pull request?

This pull request adds 2 test suites for 2 new classes HybridStore and HistoryServerMemoryManager, which were created in #28412. This pull request also did some minor changes in these 2 classes to expose some variables for testing. Besides 2 suites, this pull request adds a unit test in FsHistoryProviderSuite to test parsing logs with HybridStore.

Why are the changes needed?

Unit tests are needed for new features.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests.

baohe-zhang · 2020-08-21T16:08:00Z

cc @gatorsmile @HeartSaVioR ^^ Unit tests for #28412.

SparkQA · 2020-08-21T16:13:50Z

Test build #127750 has finished for PR 29509 at commit e5ec236.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

baohe-zhang · 2020-08-21T19:03:19Z

cc @tgravescs @redsanket ^^

SparkQA · 2020-08-21T19:21:26Z

Test build #127751 has finished for PR 29509 at commit 9043b7a.

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

SparkQA · 2020-08-21T21:35:50Z

Test build #127761 has finished for PR 29509 at commit b3eacc3.

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

HeartSaVioR

Thanks for the patch. Done the first round of review.

HeartSaVioR · 2020-08-22T02:52:38Z

core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

    // Use InMemoryStore to rebuild app store
    while (hybridStore == null) {
      // A RuntimeException will be thrown if the heap memory is not sufficient
-      memoryManager.lease(appId, attempt.info.attemptId, reader.totalSize,


IMHO, previous code looks better, as it doesn't require FsHistoryProvider to know about the details on memory manager. Is this related to the test code?

It's related to the test code, but my original thought is that passing the actual amount of memory, instead of filesize to memoryManager.lease() would make more sense. Although exposing inner details to fsHistoryProvider is not ideal. What's your opinion? Should I revert this change?

Previous code looks consistent with HistoryDiskManager. Let's leave it as it is.

If we really want to make the approximate memory calculation be separate and be exposed to others, I guess it no longer needs to be class method. It may be method of the util object, which also contain the approximate disk usage as well. (so both will keep consistent after the change) Probably better to do it in separate PR if you think it looks to be a good change.

Make sense, I've reverted the changes on lease().

HeartSaVioR · 2020-08-22T09:15:31Z

core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala


    if (!inMemory) {
      conf.set(LOCAL_STORE_DIR, Utils.createTempDir().getAbsolutePath())
+      if (useHybridStore) {


This is affected by the default value of config (suppose the default value becomes true) - we'd be better to just assign.

HeartSaVioR · 2020-08-22T09:24:10Z

core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala

+      if (switch) switchHybridStore(store)
+
+      intercept[NoSuchElementException] {
+       store.read(t2.getClass(), t2.key)


nit: indent seems to be broken

HeartSaVioR · 2020-08-22T09:39:53Z

core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala

+    val listener = new SwitchListener()
+    store.switchToLevelDB(listener, "test", None)
+    assert(listener.waitUntilDone())
+    while (!store.getStore().isInstanceOf[LevelDB]) {


Would we need to wait more after waiting listener? Or just to make sure?

There is a potential race in https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/history/HybridStore.scala#L154. So we need this while loop to ensure the shouldUseInMemoryStore is set to false.

HeartSaVioR · 2020-08-22T09:42:28Z

core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala

+    assert(store.getStore().isInstanceOf[InMemoryStore])
+    val listener = new SwitchListener()
+    store.switchToLevelDB(listener, "test", None)
+    assert(listener.waitUntilDone())


Please introduce test timeout to avoid hang on possible (future) bugs, either here or waitUntilDone().

failAfter() added here.

HeartSaVioR · 2020-08-22T09:43:31Z

core/src/test/scala/org/apache/spark/deploy/history/HybridStoreSuite.scala

+
+  private class SwitchListener extends HybridStore.SwitchToLevelDBListener {
+
+    private val results = new LinkedBlockingQueue[Int]()


Let's just simplify it - true/false would be sufficient. If in doubt we can leave one line comment to explain the meaning of true/false.

Fixed, using boolean directly.

SparkQA · 2020-08-24T06:44:40Z

Test build #127826 has finished for PR 29509 at commit 1b105e3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HybridStoreSuite extends SparkFunSuite with BeforeAndAfter with TimeLimits

SparkQA · 2020-08-24T18:30:43Z

Test build #127846 has finished for PR 29509 at commit f83d2df.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR

LGTM pending test

HeartSaVioR · 2020-08-25T00:08:19Z

retest this, please

SparkQA · 2020-08-25T03:01:37Z

Test build #127861 has finished for PR 29509 at commit f83d2df.

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

HeartSaVioR · 2020-08-25T03:09:01Z

Thanks @baohe-zhang for the PR! Merged into master.

baohe-zhang · 2020-08-25T03:39:05Z

@HeartSaVioR Thanks for the review!

Baohe Zhang added 3 commits August 21, 2020 00:37

Add unit test for HybridStore

2adfa29

Add unit test for HistoryServerMemoryManager

0232708

Fix indent

e5ec236

probot-autolabeler bot added the CORE label Aug 21, 2020

Baohe Zhang added 2 commits August 21, 2020 11:19

Fix style issue

9043b7a

Fix a race in test

b3eacc3

HeartSaVioR reviewed Aug 22, 2020

View reviewed changes

Addressed comments

1b105e3

Reverted changes in lease()

f83d2df

HeartSaVioR approved these changes Aug 25, 2020

View reviewed changes

HeartSaVioR closed this in 9151a58 Aug 25, 2020

baohe-zhang mentioned this pull request Aug 25, 2020

[SPARK-32350][FOLLOW-UP] Fix count update issue and partition the value list to a set of small batches for LevelDB writeAll #29425

Closed


		private class SwitchListener extends HybridStore.SwitchToLevelDBListener {

		private val results = new LinkedBlockingQueue[Int]()

[SPARK-31608][CORE][WEBUI][TEST] Add test suites for HybridStore and HistoryServerMemoryManager #29509

[SPARK-31608][CORE][WEBUI][TEST] Add test suites for HybridStore and HistoryServerMemoryManager #29509

Uh oh!

Conversation

baohe-zhang commented Aug 21, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

baohe-zhang commented Aug 21, 2020

Uh oh!

SparkQA commented Aug 21, 2020

Uh oh!

baohe-zhang commented Aug 21, 2020

Uh oh!

SparkQA commented Aug 21, 2020

Uh oh!

SparkQA commented Aug 21, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 24, 2020

Uh oh!

SparkQA commented Aug 24, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Aug 25, 2020

Uh oh!

SparkQA commented Aug 25, 2020

Uh oh!

HeartSaVioR commented Aug 25, 2020

Uh oh!

baohe-zhang commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HeartSaVioR Aug 24, 2020 •

edited

Loading