Skip to content

Conversation

@zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Aug 5, 2025

What changes were proposed in this pull request?

We have a log before OOM for off-heap memory allocation.

Before the change, the log is:

25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage

After:

25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage and 500 bytes of memory are used but unmanaged

Why are the changes needed?

Following #51708, to allow user to know the reason if the unmanaged memory causes OOM.

Does this PR introduce any user-facing change?

Only changes a log message.

How was this patch tested?

Existing tests.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the CORE label Aug 5, 2025
@zhztheplayer
Copy link
Member Author

@ericm-db @anishshri-db Would you like to review? Thanks!

Copy link
Contributor

@anishshri-db anishshri-db left a comment

Choose a reason for hiding this comment

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

lgtm - thanks !

@zhztheplayer
Copy link
Member Author

@anishshri-db Thank you!

"{} bytes of memory are used for execution and {} bytes of memory are used for storage",
"{} bytes of memory are used for execution " +
"and {} bytes of memory are used for storage " +
"and {} bytes of memory are used but unmanaged",
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but this might sounds misleading. Can we revise this like the following, @zhztheplayer ?

- and {} bytes of memory are used but unmanaged
+ and {} bytes of unmanaged memory are used

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi dongjoon-hyun, just to clarify, I guess you were suggesting to only change the 3rd sentence here?

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @zhztheplayer and @anishshri-db .

Merged to master/4.1 for Apache Spark 4.1.0.

dongjoon-hyun pushed a commit that referenced this pull request Nov 3, 2025
…efore execution memory OOM

### What changes were proposed in this pull request?

We have a log before OOM for off-heap memory allocation.

Before the change, the log is:

> 25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage

After:

> 25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage and 500 bytes of memory are used but unmanaged

### Why are the changes needed?

Following #51708, to allow user to know the reason if the unmanaged memory causes OOM.

### Does this PR introduce _any_ user-facing change?

Only changes a log message.

### How was this patch tested?

Existing tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #51848 from zhztheplayer/wip-53128.

Authored-by: Hongze Zhang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit c4ad381)
Signed-off-by: Dongjoon Hyun <[email protected]>
@zhztheplayer
Copy link
Member Author

Thank you for reviewing!

huangxiaopingRD pushed a commit to huangxiaopingRD/spark that referenced this pull request Nov 25, 2025
…efore execution memory OOM

### What changes were proposed in this pull request?

We have a log before OOM for off-heap memory allocation.

Before the change, the log is:

> 25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage

After:

> 25/08/05 16:44:32 INFO TaskMemoryManager: 100 bytes of memory are used for execution and 100 bytes of memory are used for storage and 500 bytes of memory are used but unmanaged

### Why are the changes needed?

Following apache#51708, to allow user to know the reason if the unmanaged memory causes OOM.

### Does this PR introduce _any_ user-facing change?

Only changes a log message.

### How was this patch tested?

Existing tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#51848 from zhztheplayer/wip-53128.

Authored-by: Hongze Zhang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants