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

Implements an ArrowRootAllocationProvider SPI #1040

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

drewfarris
Copy link
Collaborator

Other utilities that run within Emissary would benefit from a central source for getting access to an Arrow RootAllocator. This PR implements the ArrowRootAllocationProvider that creates a new RootAllocator and provides a static method for retrieving that instance. It also properly handles calling close() on the allocator at shutdown time. It can be initialized by including it in the META-INF/service/emissary.spi.InitializationProvider resource file.

In the current state, we leave it up to the consumer to decide whether to create a child allocator using the reference to the root allocator, or to use the root allocator directly. We could decide to force consumers to create child allocators instead of providing references to the root allocator, but I'm not convinced that it is worth doing that at this point.

See also:

@drewfarris drewfarris changed the title Implements an ArrowRootAllocationProfider SPI Implements an ArrowRootAllocationProvider SPI Jan 7, 2025
@drewfarris drewfarris force-pushed the feature/arrow-root-allocation-provider branch from db6c2f1 to 7e9b903 Compare January 7, 2025 14:03
pom.xml Outdated
<exclusion>
<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
</exclusion>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not totally certain this is the best way to approach this, but it's a good point for discussion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is marked outdated because I moved the exclusion to the dependencyManagement section, but it's still relevant to discuss.

@drewfarris
Copy link
Collaborator Author

This may need additional work. After doing some brief testing, if someone creates a child allocator from the root allocator, and that child allocator is not properly closed, calling shutdown on the provider seems to raise an IllegalStateException. Changing this to a draft PR in the meantime.

@drewfarris drewfarris self-assigned this Jan 7, 2025
@drewfarris drewfarris marked this pull request as draft January 7, 2025 14:10
@drewfarris drewfarris marked this pull request as ready for review January 9, 2025 20:17
@jpdahlke jpdahlke added this to the v8.21.0 milestone Jan 10, 2025
@drewfarris
Copy link
Collaborator Author

This may need additional work. After doing some brief testing, if someone creates a child allocator from the root allocator, and that child allocator is not properly closed, calling shutdown on the provider seems to raise an IllegalStateException. Changing this to a draft PR in the meantime.

The latest revision addresses this.

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.

2 participants