Skip to content

[#2411] fix(spark): Avoid double release memory when trigger spill#2412

Closed
summaryzb wants to merge 1 commit intoapache:masterfrom
summaryzb:master
Closed

[#2411] fix(spark): Avoid double release memory when trigger spill#2412
summaryzb wants to merge 1 commit intoapache:masterfrom
summaryzb:master

Conversation

@summaryzb
Copy link
Contributor

What changes were proposed in this pull request?

When trigger spill, return 0 instead of the memory number of sent block.

Why are the changes needed?

Fix #2411

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT

@github-actions
Copy link

Test Results

 2 945 files   -  66   2 945 suites   - 66   9h 43m 8s ⏱️ + 2h 58m 31s
 1 147 tests  -  30   1 142 ✅  -  34   1 💤 ±0   2 ❌ + 2   2 🔥 + 2 
14 666 runs   - 248  14 611 ✅  - 288  15 💤 ±0  20 ❌ +20  20 🔥 +20 

For more details on these failures and errors, see this check.

Results for commit 4d4a5d5. ± Comparison against base commit 3874f15.

This pull request removes 30 tests.
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateFallback
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriver
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriverDenied
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInExecutor
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testDefaultIncludeExcludeProperties
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testExcludeProperties
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testIncludeProperties
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testTryAccessCluster
org.apache.spark.shuffle.FunctionUtilsTests ‑ testOnceFunction0
org.apache.spark.shuffle.RssShuffleManagerTest ‑ testCreateShuffleManagerServer
…

@roryqi roryqi requested a review from zuston March 18, 2025 02:57
// ignore
}
}
return 0L;
Copy link
Member

Choose a reason for hiding this comment

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

From my point, the return value is to tell spark memory manager we have released, that will not duplicate release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, but does it really necessary to block here to calculate memory released. since the free memory is called in multiply callback thread

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I havn't fully gotten your thought. The returned free memory size is to tell spark the spilled size.

If you don't mind, please provide more details?

Copy link
Contributor Author

@summaryzb summaryzb Mar 19, 2025

Choose a reason for hiding this comment

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

free memory is called in multiply callback thread

freeMemory of MemoryConsumer is called in multiply callback thread, but used in MemoryConsumer is a normal variable, There is a high probability that concurrent issue happens

Copy link
Member

Choose a reason for hiding this comment

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

Emm. I have to say that the spill only will be triggered by self.


if (!memorySpillEnabled || trigger != this) {
      return 0L;
    }

@summaryzb summaryzb closed this Mar 18, 2025
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.

[Bug][Spark][Client] Spill memory corresponding to successfully sent blocks

2 participants