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

feat: adapative page hints (client-side only) #1357

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

x04
Copy link
Contributor

@x04 x04 commented Oct 30, 2023

Description of change

Adds adaptive page hints to cargo-shuttle by adding 1 to the specified limit and checking if we reach that adjusted limit. This is not breaking as it requires no API changes and is just a small change to client-side logic. Only thing worth mentioning is technically you can only specify a limit up u32::MAX - 1 because we always adjust the limit to be limit + 1 however I cannot really see a case where someone would ever need that high of a limit so it is basically irrelevant.

Closes issue #1316
Also can close my other 2 PRs #1334 and #1320 as this resolves all the issues addressed in those.

How has this been tested? (if applicable)

Tested with local setup

@x04
Copy link
Contributor Author

x04 commented Oct 30, 2023

Only check that failed was an out of date crate and I can update that but will wait for confirmation that is the path to take.

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Thanks for being patient and arriving at this nice solution! Much cleaner than the others :)
I added some small changes.
PS. you can ignore the audit warning

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Great, thank you! 🥳

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