Skip to content

Zero-copy local deep storage.#13394

Merged
cheddar merged 7 commits intoapache:masterfrom
gianm:local-nozip
Dec 13, 2022
Merged

Zero-copy local deep storage.#13394
cheddar merged 7 commits intoapache:masterfrom
gianm:local-nozip

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Nov 21, 2022

Reduces disk usage and makes Historicals able to load segments instantaneously.

Two changes:

  1. Introduce "druid.storage.zip" parameter for local storage, which defaults
    to false. This changes default behavior from writing an index.zip to writing
    a regular directory. This is safe to do even during a rolling update, because
    the older code actually already handled unzipped directories being present
    on local deep storage.

  2. In LocalDataSegmentPuller and LocalDataSegmentPusher, use hard links
    instead of copies when possible. (Generally this is possible when the
    source and destination directory are on the same filesystem.)

This is useful for local deep storage, since it reduces disk usage and
makes Historicals able to load segments instantaneously.

Two changes:

1) Introduce "druid.storage.zip" parameter for local storage, which defaults
   to false. This changes default behavior from writing an index.zip to writing
   a regular directory. This is safe to do even during a rolling update, because
   the older code actually already handled unzipped directories being present
   on local deep storage.

2) In LocalDataSegmentPuller and LocalDataSegmentPusher, use hard links
   instead of copies when possible. (Generally this is possible when the
   source and destination directory are on the same filesystem.)
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging 8a388f1 into a860baf - view on LGTM.com

new alerts:

  • 2 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Nov 21, 2022

2 for Uncontrolled data used in path expression

LGTM traced this back to a Task object, which is safe because task IDs are path-safe thanks to IdUtils.validateId.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging 8906ebd into fa3ab27 - view on LGTM.com

new alerts:

  • 2 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging e72aaca into 133054b - view on LGTM.com

new alerts:

  • 2 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 22, 2022

This pull request introduces 2 alerts when merging e726833 into fe34ecc - view on LGTM.com

new alerts:

  • 2 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

|`druid.storage.storageDirectory`||Directory for storing segments.|Must be set.|
|`druid.storage.type`|`local`||Must be set.|
|`druid.storage.storageDirectory`|any local directory|Directory for storing segments. Must be different from `druid.segmentCache.locations` and `druid.segmentCache.infoDir`.|`/tmp/druid/localStorage`|
|`druid.storage.zip`|`true`, `false`|Whether segments in `druid.storage.storageDirectory` are written as directories (`false`) or zip files (`true`).|`false`|
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.

Personally, I'd be down for just making the segment storage stop zipping up the files. It'd be good for a wide range of things, I think. The only risk is that it might create extra data on deep storage for people, but I'm not sure that's really the top-of-mind concern for most people.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made this an option for risk-reduction purposes, and would prefer to keep it an option for now for that reason, but I am definitely down to make it always-not-zipped in the not-too-distant future.

Comment thread docs/dependencies/deep-storage.md Outdated
Comment on lines +50 to +53
When using local storage, it is recommended to use the same filesystem for `druid.storage.type` and
`druid.segmentCache.locations` on your Historicals. When these are on the same filesystem, and
`druid.storage.zip = false` (the default), Druid creates hard links between the two directories rather than storing
two copies of the same data.
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.

Personally, I wish that this would call out that manual setting of local storage in a production environment is an "advanced feature" that should be done intentionally. Or maybe just a first sentence that pushes for deep storage more. Something like

Use of a cloud store is recommended for deep storage in any production system.  If you know what you are doing and are using local storage, consider setting the same filesystem ...

I know the paragraph below calls out the cloud stuff as well, I don't think it hurts to mention it multiple times.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was weirdly tough to edit the docs in a way that felt right. I ended up moving the "you should use cloud storage in prod" to the top, and deleting the bottom section about hard links. It started to get tough to talk about it while maintaining the appropriate level of "don't actually do this in prod". I don't think that section of the doc added much, so I felt OK removing it.

Comment thread docs/dependencies/deep-storage.md Outdated
`druid.storage.zip = false` (the default), Druid creates hard links between the two directories rather than storing
two copies of the same data.

Local storage cannot be used if you have multiple servers without a shared filesystem. In this case, consider using
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.

Consider "Local storage requires a shared file system when used across multiple servers."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After reflection, I ended up removing this section.


@Override
public Map<String, Object> makeLoadSpec(URI finalIndexZipFilePath)
private DataSegment pushNoZip(final File inDir, final File outDir, final DataSegment baseSegment) throws IOException
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.

I'm just curious about the pushing. If the push creates a hard link to an ephemeral location that belonged to the task and then the task directory is deleted, what happens to that hard link? Does the file continue to exist and only be linked from one location?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Dec 9, 2022

Choose a reason for hiding this comment

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

Yep, that is exactly how hard links work. Each one is just as good as all the other ones, none is more "real" than the others once multiple are created.

@cheddar cheddar merged commit de5a4ba into apache:master Dec 13, 2022
vogievetsky pushed a commit to vogievetsky/druid that referenced this pull request Dec 14, 2022
* Zero-copy local deep storage.

This is useful for local deep storage, since it reduces disk usage and
makes Historicals able to load segments instantaneously.

Two changes:

1) Introduce "druid.storage.zip" parameter for local storage, which defaults
   to false. This changes default behavior from writing an index.zip to writing
   a regular directory. This is safe to do even during a rolling update, because
   the older code actually already handled unzipped directories being present
   on local deep storage.

2) In LocalDataSegmentPuller and LocalDataSegmentPusher, use hard links
   instead of copies when possible. (Generally this is possible when the
   source and destination directory are on the same filesystem.)
@kfaraz kfaraz added this to the 25.0 milestone Dec 14, 2022
kfaraz pushed a commit that referenced this pull request Dec 14, 2022
* Zero-copy local deep storage.

This is useful for local deep storage, since it reduces disk usage and
makes Historicals able to load segments instantaneously.

Two changes:

1) Introduce "druid.storage.zip" parameter for local storage, which defaults
   to false. This changes default behavior from writing an index.zip to writing
   a regular directory. This is safe to do even during a rolling update, because
   the older code actually already handled unzipped directories being present
   on local deep storage.

2) In LocalDataSegmentPuller and LocalDataSegmentPusher, use hard links
   instead of copies when possible. (Generally this is possible when the
   source and destination directory are on the same filesystem.)

Co-authored-by: Gian Merlino <gianmerlino@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants