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

Update mock-mount-s3 to require '--max-throughput-gbps' argument #1018

Merged

Conversation

dannycjones
Copy link
Contributor

Description of change

It was unclear to me when first using mock-mount-s3 that it will limit throughput based on the value for --max-throughput-gbps, and that this will default to 10Gbps as real Mountpoint does if it can't detect the correct value.

This change requires the argument to be specified, so that the user can be forced to acknowledge this default or choose a better value for the use case.

Relevant issues: N/A

Does this change impact existing behavior?

It does, but only for the mock binary. You now must specify --max-throughput-gbps <Gbps>.

Does this change need a changelog entry in any of the crates?

No.


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).

Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>
Copy link
Contributor

@monthonk monthonk left a comment

Choose a reason for hiding this comment

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

Does this binary really compile? I have to add features = ["thread-pool"] for the futures crate to make it work. Maybe we should also fix that.

mountpoint-s3/src/bin/mock-mount-s3.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>
@dannycjones
Copy link
Contributor Author

dannycjones commented Sep 24, 2024

Does this binary really compile? I have to add features = ["thread-pool"] for the futures crate to make it work. Maybe we should also fix that.

Yeah, I wasn't sure on the best practice here. I've submitted a fix in #1030.

Copy link
Contributor

@monthonk monthonk left a comment

Choose a reason for hiding this comment

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

Sounds good! thanks Danny.

@dannycjones dannycjones added this pull request to the merge queue Sep 24, 2024
Merged via the queue into awslabs:main with commit ed8d96b Sep 24, 2024
23 checks passed
@dannycjones dannycjones deleted the mock-client-require-throughput-arg branch September 24, 2024 09:52
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