Skip to content

Move shared cache pre-allocation and support macOS #70285

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

Merged
merged 12 commits into from
Mar 11, 2021

Conversation

jasontedor
Copy link
Member

This commit moves the shared cache pre-allocation code out of bootstrap, and instead to the searchable snapshots code. We got out of our way to only grant permissions to a specific library used for the pre-allocation.

Additionally, to ensure our interfaces are sound, we add a macOS implementation based on fcntl and ftruncate.

Relates #68858

This commit moves the shared cache pre-allocation code out of bootstrap,
and instead to the searchable snapshots code. We got out of our way to
only grant permissions to a specific library used for the
pre-allocation.

Additionally, to ensure our interfaces are sound, we add a macOS
implementation based on fcntl and ftruncate.
@jasontedor
Copy link
Member Author

The main purpose of this relocation is to keep this plugin-specific code out of bootstrap. Additionally, for some complicated reasons, we can't touch the roles until after plugins are done being loaded. This means that we can't make the validation fo these settings dependent on the node roles unless we relocate these settings out of bootstrap so that they are not touched until after plugins are done being loaded. This gives us a second reason to move this code, so we take those steps now.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

The plugin+security policy side LGTM.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've done a first pass, and pushed a commit to fix precommit (as best I could).

I'm wondering if there's some test we can add to make sure this actually works on MacOS / Linux besides manual testing.

@jasontedor
Copy link
Member Author

@ywelsch Thanks for making a pass, and especially for addressing precommit. 🙏

I took your suggestion to first attempt to allocate contiguously on macOS.

I agree on adding testing, I'll do that in a follow-up.

@jasontedor jasontedor requested a review from ywelsch March 11, 2021 11:38
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

fst.fst_length = new NativeLong(fileSize);
// first, try allocating contiguously
if (Natives.fcntl(fd, Natives.Fcntl.F_PREALLOCATE, fst) != 0) {
// that failed, so let us try allocating non-contigulsouly
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: non-contiguously

@jasontedor jasontedor merged commit 36683a4 into elastic:master Mar 11, 2021
@jasontedor jasontedor added :Core/Infra/Core Core issues without another label :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.12.0 v7.13.0 v8.0.0 labels Mar 11, 2021
@elasticmachine elasticmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Core/Infra Meta label for core/infra team labels Mar 11, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 11, 2021
This commit moves the shared cache pre-allocation code out of bootstrap,
and instead to the searchable snapshots code. We go out of our way to
only grant permissions to a specific library used for the
pre-allocation.

Additionally, to ensure our interfaces are sound, we add a macOS
implementation based on fcntl and ftruncate.

Co-authored-by: Yannick Welsch <[email protected]>
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 11, 2021
This commit moves the shared cache pre-allocation code out of bootstrap,
and instead to the searchable snapshots code. We go out of our way to
only grant permissions to a specific library used for the
pre-allocation.

Additionally, to ensure our interfaces are sound, we add a macOS
implementation based on fcntl and ftruncate.

Co-authored-by: Yannick Welsch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Core/Infra Meta label for core/infra team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.12.0 v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants