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

Remove the temporary caching feature flag #622

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Nov 21, 2023

Description of change

Remove the "caching" build feature. With this change, the new command line flag --cache and related options to support caching are generally available.

Relevant issues: #255

Does this change impact existing behavior?

No. The new caching behavior is optional and needs to be explicitly enabled with a command line flag.


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 had a problem deploying to PR integration tests November 21, 2023 17:56 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 21, 2023 17:56 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 21, 2023 17:56 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 21, 2023 17:56 — with GitHub Actions Failure
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 18:00 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 18:00 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 18:00 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 18:00 — with GitHub Actions Inactive
@dannycjones dannycjones self-requested a review November 21, 2023 18:54
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.

Small note on changelog.

Code changes LGTM.

* No breaking changes.

### Other changes
* Introduced the `--cache` option to configure Mountpoint to cache object metadata and content, in order to reduce cost and improve performance for repeated reads to the same files. ([#622](https://github.com/awslabs/mountpoint-s3/pull/622))
Copy link
Contributor

@dannycjones dannycjones Nov 21, 2023

Choose a reason for hiding this comment

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

I think this is a bit understated. Should it be under something like ### New features?

In terms of the text, maybe something like:

### New features

Introduced optional caching of object metadata and content,
in order to allow reduced cost and improved performance for repeated reads to the same files.
([#622](https://github.com/awslabs/mountpoint-s3/pull/622))

I'm also thinking when we merge the documentation, we can follow up too:

### New features

Introduced optional caching of object metadata and content,
in order to allow reduced cost and improved performance for repeated reads to the same files.
To get started, see the [caching section of the configuration documentation](https://github.com/awslabs/mountpoint-s3/blob/main/doc/CONFIGURATION.md#caching-configuration).
([#622](https://github.com/awslabs/mountpoint-s3/pull/622))

Copy link
Contributor

Choose a reason for hiding this comment

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

@jamesbornholt can you share feedback here too?

Copy link
Member

Choose a reason for hiding this comment

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

agree we should put this in a new features section

mountpoint-s3/Cargo.toml Show resolved Hide resolved
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 November 21, 2023 19:40 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 19:40 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 19:40 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 21, 2023 19:40 — with GitHub Actions Inactive
@passaro
Copy link
Contributor Author

passaro commented Nov 21, 2023

Adopted @dannycjones suggestions and rebased to solve conflicts with main.

@passaro passaro marked this pull request as ready for review November 21, 2023 19:42
@dannycjones dannycjones added this pull request to the merge queue Nov 22, 2023
Merged via the queue into awslabs:main with commit 6d5bb1f Nov 22, 2023
18 checks passed
@passaro passaro deleted the caching-feature branch November 30, 2023 15:11
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