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

Allow --metadata-ttl without --cache and set default with --cache to 60s #855

Merged
merged 6 commits into from
May 1, 2024

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Apr 17, 2024

Description of change

Allow users to explicitly enable metadata caching, independently of object content caching, by setting the --metadata-ttl option. Before this change, users were required to also set --cache <DIR> in order to configure the metadata TTL. To enable just the metadata cache they had to resort to the workaround of restricting the cache size to zero (--max-cache-size 0).
We are also introducing the following special values for --metadata-ttl:

  • minimal: describe the default setting (when --cache is not used) where Mountpoint caches metadata for up to 1s, refreshes it on open, and does not keep a negative cache.
  • indefinite: new value recommended for users with static buckets, disables refreshing of metadata. (Implemented by setting the TTL to the maximum allowed value, ~100 years).

When --cache is set, we are now also setting the default TTL for metadata to 60s (1min) vs the previous 1s.

Finally, setting --metadata-ttl 0 now resolves to minimal and logs a warning to notify users.

The caching options section looks like this:

Caching options:
      --cache <DIRECTORY>
          Enable caching of object content to the given directory and set metadata TTL to 60s
      --metadata-ttl <SECONDS|indefinite|minimal>
          Time-to-live (TTL) for cached metadata in seconds [default: minimal, or 60s if --cache is set]
      --max-cache-size <MiB>
          Maximum size of the cache directory in MiB [default: preserve 5% of available space]

Relevant issues: #752, #759, #768

Does this change impact existing behavior?

Two changes to existing behavior:

  1. When setting --cache but not --metadata-ttl, the default TTL is 60s vs the previous 1s.
  2. Setting --metadata-ttl 0 resolves to minimal and logs a warning.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 17, 2024 12:55 — with GitHub Actions Inactive
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

Looks good!

Before merging:

mountpoint-s3/tests/cli.rs Outdated Show resolved Hide resolved
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

Just some minor stuff

mountpoint-s3/src/cli.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/cli.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/fs.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/lib.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/time_to_live.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/time_to_live.rs Outdated Show resolved Hide resolved
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro had a problem deploying to PR integration tests April 18, 2024 10:05 — with GitHub Actions Failure
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 10:05 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
@passaro passaro had a problem deploying to PR integration tests April 18, 2024 12:37 — with GitHub Actions Failure
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 18, 2024 12:37 — with GitHub Actions Inactive
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

One minor suggestion, and then needs a rebase.

mountpoint-s3/src/cli.rs Outdated Show resolved Hide resolved
passaro added 5 commits April 30, 2024 09:02
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:23 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:23 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:23 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:23 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:24 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:24 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests April 30, 2024 14:24 — with GitHub Actions Inactive
@passaro passaro marked this pull request as ready for review April 30, 2024 14:31
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

LGTM, just one changelog thing.

Comment on lines 3 to 4
### Breaking changes
* No breaking changes.
* When using the `--cache` flag, the default metadata time-to-live (TTL) is now set to 60 seconds instead of 1 second. The `--metadata-ttl` flag can still be set to further configure it, but can now also be used without the `--cache` flag in order to only enable caching of metadata. In addition to values in seconds, `--metadata-ttl` can now also be set to `minimal` (strict consistency) or `indefinite` (no updates). The `--metadata-ttl 0` setting is no longer supported, is now interpreted as `minimal`, and will be removed in a future release. ([#855](https://github.com/awslabs/mountpoint-s3/pull/855))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe split this up because only part of it is breaking

New features

  • Metadata caching can now be configured independently of data caching. When passing the --metadata-ttl <seconds> argument without also specifying --cache <directory>, Mountpoint will cache file metadata in memory for up to the given TTL, but will not cache object data. The --metadata-ttl argument also accepts two special values: minimal to enable only the minimal necessary caching, and indefinite to cache indefinitely. These modes can help accelerate workloads that touch many files but do not need to cache object data for re-use (for example, listing a directory and then reading each file within it once).

Breaking changes

  • The --metadata-ttl 0 setting is no longer supported and will be removed in a future release. The new --metadata-ttl minimal has a similar effect, but behaves better when latency for S3 requests is high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also added a breaking change entry for the new default TTL with cache.

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests May 1, 2024 12:50 — with GitHub Actions Inactive
@jamesbornholt jamesbornholt added this pull request to the merge queue May 1, 2024
Merged via the queue into awslabs:main with commit e32f890 May 1, 2024
23 checks passed
@passaro passaro deleted the caching-options branch May 13, 2024 19:47
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.

3 participants