-
Notifications
You must be signed in to change notification settings - Fork 39
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
Retry mechanism for sbt #450
Conversation
someone can please approve the workflow? :) |
trying again .. |
modules/lm-coursier/src/main/scala/lmcoursier/internal/ResolutionRun.scala
Outdated
Show resolved
Hide resolved
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.
Overall LGTM
Would you like to squash your commits? Otherwise, I'll squash and merge. |
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.
Sorry for blocking the merge until further discussion here, but there are a number of issues in the approach of that PR. I don’t think retrying the whole resolution whenever it failed is the right thing to do. As your link to the Gradle docs describes, Gradle only retries things for individual downloads, and only so for specific error types. Coursier already does that for some TLS-related errors, and that’s the approach that should be extended IMO (I recall several @eed3si9n’s comments going in the same direction)
@hagay3 if ever you feel… too intimidated by the coursier codebase or don’t have too much time to dive into it, and really want to add that here, that might work, but the errors that trigger retries really have to be more specific. Currently, it seems any kind of error triggers a retry, which is too broad (we don’t want to retry after 404 errors for example, which is what worried me here). I’d advice maybe to have a look at the output of a resolution that you would have liked to see retried, look for the specific exception that made it fail (like a ConnectException for example), and make only those trigger a retry here. |
Thank you for your feedback |
yes please squash my branch :) |
looks good, would love to have that! |
@alexarchambault |
Sorry for the delay, I'll try to find time for this in the coming days |
Hi, any updates regarding that? |
it's been a while since there was any update here. |
@eed3si9n @alexarchambault |
@eed3si9n @alexarchambault |
That method also has the benefit of setting a keep-alive time for its threads, so that the scheduler should automatically shutdown after being unused for some delay (currently 1 minute)
52d3e49
to
e19428b
Compare
@hagay3 I rebased, tweaked many small things, and changed the way the error is detected (ensuring we retry iff a connection timeout is detected) Should be good to merge to me, but for the last commit that I need to revert |
…diff" This reverts commit e19428b.
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.
Merging, thanks @hagay3!
Should eliminate the sporadic connection failures when downloading dependencies. The fix that add retries has been implemented in latest `coursier` coursier/sbt-coursier#450.
Should eliminate the sporadic connection failures when downloading dependencies. The fix that adds retries has been implemented in latest `coursier` coursier/sbt-coursier#450. # Important Notes For example https://github.com/enso-org/enso/actions/runs/7132038630/job/19421764930?pr=8496
Thanks @alexarchambault ! |
A retry mechanism for sbt
Resolves coursier/coursier#2733 and sbt/sbt#7200
Gradle implementation
https://docs.gradle.org/current/userguide/dependency_resolution.html#sub:http-retries
Documentation and tests will be added with more guidance
@alexarchambault @eed3si9n let me know how to proceed :)