Skip to content

fix: expose native memory usage to Spark - #5408

Merged
sunchao merged 2 commits into
apache:mainfrom
ywskycn:dev/codex/comet-expose-native-memory-usage
Aug 24, 2026
Merged

sunchao merged 2 commits into
apache:mainfrom
ywskycn:dev/codex/comet-expose-native-memory-usage

Conversation

@ywskycn

@ywskycn ywskycn commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Resolving the sub issue 5 in #5212

Rationale for this change

What changes are included in this PR?

How are these changes tested?

@ywskycn
ywskycn force-pushed the dev/codex/comet-expose-native-memory-usage branch from 18738e8 to c2d112f Compare August 21, 2026 15:42

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ywskycn this looks useful, however it would be nice to find a consumer on Scala side and also check if this value accounted properly

@ywskycn

ywskycn commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Thx @comphead . Updated with a unit test. Anything else you think we should include?

@ywskycn
ywskycn force-pushed the dev/codex/comet-expose-native-memory-usage branch from 5f4cb6c to 3e52b6b Compare August 24, 2026 19:09
@sunchao
sunchao merged commit f5d49de into apache:main Aug 24, 2026
71 checks passed
@sunchao

sunchao commented Aug 24, 2026

Copy link
Copy Markdown
Member

Merged, thanks @ywskycn ! @comphead let me know if you have more comments on this PR. We can add follow-ups.

andygrove added a commit that referenced this pull request Sep 29, 2026
#6269) (#6419)

CometTaskMemoryManager.acquireMemory logged a warning, then called
TaskMemoryManager.showMemoryUsage, every time Spark granted less memory than a
native pool asked for. A partial grant is how a native operator learns to spill,
so a query that spills logged hundreds of them. Log the partial grant at DEBUG
instead. A refusal that fails the task already says in its error what Spark
granted and lists the pool's top consumers.

Drop the dump rather than moving it to DEBUG. showMemoryUsage takes the
TaskMemoryManager monitor. Another acquire of the same task can hold that
monitor while it waits inside Spark for memory, and this thread still holds its
partial grant. The task then hangs until some other task frees memory.
greedy_unified can reach this on main, because it calls Spark without a lock.
sunchao found the cycle while reviewing #5613.

Closes #6257.

Adapted for branch-1.0: CometTaskMemoryManagerSuite does not exist here,
because main added it in #5408. It is added with only this fix's two tests
and the helpers they use, and listed in both PR workflows. The suite's other
tests on main check a NativeMemoryConsumer.getUsed override from #5408.

(cherry picked from commit e1d2c11)
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.

3 participants