Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Require offHeap memory to be enabled (always use unified memory) #1062

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented Nov 6, 2024

Which issue does this PR close?

Closes #949
Closes #1017

Rationale for this change

What changes are included in this PR?

  • Fall back to Spark if offHeap is disabled
  • Update tuning guide

How are these changes tested?

@andygrove andygrove changed the title feat: Require offHeap memory to be enabled feat: Require offHeap memory to be enabled (always use unified memory) Nov 6, 2024
@parthchandra
Copy link
Contributor

Is #875 a blocker for this PR

@andygrove
Copy link
Member Author

Is #875 a blocker for this PR

I don't think so. I have been using the unified memory approach for my local benchmarking and have not encountered any issues.

@andygrove
Copy link
Member Author

All tests are passing, but this is misleading because the Spark tests are running with spark.memory.offHeap.enabled=false, so they are not using Comet for execution.

@viirya
Copy link
Member

viirya commented Nov 7, 2024

Okay. At least we know Spark tests can pass under off-heap. It is a good news for us. We don't need to update Spark tests. In #1063, we can restore it to test Comet execution.

Oh, I misread it.

@viirya
Copy link
Member

viirya commented Nov 7, 2024

Yea, we can try to restore it in #1063. As I commented there, if Spark tests cannot pass under off-heap, we can have a test-only setup to run Spark tests with on-heap + Comet execution.

@andygrove andygrove marked this pull request as ready for review November 14, 2024 15:15
@andygrove andygrove merged commit 2c832b4 into apache:main Nov 14, 2024
76 checks passed
@andygrove andygrove deleted the issue-1017 branch November 14, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants