-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 DEDUPLICATE_BUILDS feature flag #7306
Conversation
We have been testing this for some weeks now and we haven't seen any issue with it. We are ready to remove the feature flag and make this the default behavior.
You can also do a final test by making the flag |
Yea, let's set the feature flag for a week before we fully remove it. |
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.
Waiting on setting the feature flag to historical_deafult_true for a week.
I have marked this one as default true today. |
@stsewd @ericholscher are we ready to merge this PR now? |
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.
There were some problems with one of our repos, not sure if we want so solve that first.
Yea, I feel like without being able to cancel a build, you can get stuck in a state where it's getting deduped but the build failed and didn't report status. Pretty bad UX, which was happening on test-builds. |
I'm going to mark this PR as blocked for now since it will have a big impact and could bring issues to some projects. |
This is still a good improvement to reduce our servers load, but we need a smarter "Finish inactive builds" first. I experimented with this at #8269 |
We had implemented the ability to cancel a build now. Should we solve the conflicts and try to move forward with this PR now? The first test would be to re-enable |
@humitos yep. Thought we did hit some issues with cancelling builds not actually cancelling them in the UX, which made users struggle with their concurrency limit. |
Today, while taking a look at #8961I checked if the I think this is currently working fine because we added the condition that only applies to builds triggered 5 minutes ago only: readthedocs.org/readthedocs/core/utils/__init__.py Lines 136 to 142 in 877c3ba
|
This PR won't probably be needed if we move forward with #9549 😄 |
We have been testing this for some weeks now and we haven't seen any issue with
it. We are ready to remove the feature flag and make this the default behavior.