Uh oh!
There was an error while loading. Please reload this page.
tests: only refresh the minimum sysinfo in mem limit tests. - #15702
Conversation
ashdnazg
commented
Apr 14, 2025
Can we run the extended tests on this PR? |
jayzhan211
commented
Apr 14, 2025
Run extended tests |
ashdnazg
commented
Apr 14, 2025
@jayzhan211https://github.com/apache/datafusion/actions/runs/14439917392/job/40487622886 seems to have failed? |
jayzhan211
commented
Apr 14, 2025
it is still running |
ashdnazg
commented
Apr 14, 2025
I don't think it does, the spawning crashed, there's nothing running here: https://github.com/apache/datafusion/actions?query=workflow%3A%22Datafusion+extended+tests%22 |
ashdnazg
commented
Apr 14, 2025
The CI issue is hopefully fixed here: #15704 |
2010YOUY01
commented
Apr 14, 2025
Thank you for the fix. To test this PR, I think you can push the change to your cloned repository of |
ashdnazg
commented
Apr 14, 2025
Tests seem to finish nicely: https://github.com/ashdnazg/datafusion/actions/runs/14442804912/job/40496805764 |
alamb
commented
Apr 14, 2025
Thank you @ashdnazg and @jayzhan211 |
Rationale for this change
Currently the memory limit tests hang due to lock contention and the overhead from frequently calling
sysinfo::System::refresh_all()in the memory monitoring task.In fact, we don't really need to refresh all info, it's enough to just get the memory data about the current process, which significantly reduces the refresh time and the tests finish quickly.
What changes are included in this PR?
In the memory limit utils, the memory monitoring spawned task now refreshes system info only for what's actually necessary.
Are these changes tested?
It's part of the tests, and they seem to not hang anymore :)
Are there any user-facing changes?
No