-
Notifications
You must be signed in to change notification settings - Fork 114
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
Drop server performance configuration #877
Conversation
M0rgan01
commented
Sep 6, 2024
Questions | Answers |
---|---|
Description? | Drop server performance configuration |
Type? | improvement |
BC breaks? | no |
Deprecations? | no |
Fixed ticket? | - |
Sponsor company | - |
How to test? | - |
a6df5cf
to
6fe8356
Compare
6fe8356
to
815b581
Compare
Are you planning to allow developers to override these values? Time is precious. |
@kpodemski According to our metrics this configuration was very little modified. We aim to simplify the update process so that users are as comfortable as possible. Time is precious -> We also study the subject of performance :) |
Of course, and that's a good approach. Two thoughts:
$nbFiles = Configuration::get('PS_AUTOUPGRADE_NB_FILES, null, null, null, 400); so I could tinker with perf settings :D |
I don't think the loop size/time is to concern, if the upgrade takes 2 minutes o 2 minutes 5 seconds is irrelevant. What is concerning is |
This configuration does not significantly change the time an upgrade will take. This can also complicate things if we try to improve performance for all configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @M0rgan01
Thank you for your PR, I tested it and it seems to works as you can see :
recording.287.webm
Tested from :
1.7.8.9 to 8.1.7
8.0.5 to 8.1.7
8.1.7 to 9.0.0
8.1.5 to 9.0.0
8.0.5 to 9.0.0
Because the PR seems to works as expected, It's QA ✔️
Thank you