-
Notifications
You must be signed in to change notification settings - Fork 753
Include retries in git repo requests #6121
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
Include retries in git repo requests #6121
Conversation
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
christopher-hakkaart
left a comment
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.
Documentation changes look great.
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
|
I have added
There are a few other places that use retry settings, namely the grid and cloud executors. In a future iteration I think we could consolidate them as well, perhaps using the |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
…ty-failures-in-automated-workflows
b4b321e to
069653d
Compare
…ty-failures-in-automated-workflows
| // Other methods can throw IOExceptions including for 50x response codes which are retried by default. | ||
| if( t instanceof RateLimitExceededException || t.cause instanceof RateLimitExceededException) | ||
| return true | ||
| if( t instanceof IOException || t.cause instanceof IOException ) |
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.
IOException is too generic. It would be preferable having a more granular control on http error codes here.
However it can be difficult to achieve this in the current implementation. Wonder if it should be used a different http client (see WaveClient)
…ty-failures-in-automated-workflows
|
This is the PR implementing the use of Java HttpClient #6188 |
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
| try { | ||
| def config = getConfig0() | ||
| def manifestOpts = config.manifest as Map ?: Collections.emptyMap() | ||
| return new Manifest(manifestOpts) | ||
| } | ||
| catch( Exception e ) { | ||
| log.warn "Cannot read project config -- Cause: ${e.message ?: e}" | ||
| return new Manifest(Collections.emptyMap()) | ||
| } |
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.
Likely this is not needed anymore, because the retry strategy works at a lower level
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.
what do you mean?
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
|
Since there are several overlapping changes I've created a new PR. Closing this in favour of #6195 |
This PR include retries when performing request in git repositories.
The request errors are managed at
RepositoryProvider.checkResponse, where someAbortExceptionorRateLimitExceededExceptionsare thrown for some HTTP error codes, or by IOExceptionsthrown by theURLConnectionmethods. They includeSocketExceptionsand50Xerror codes. I have made allIOExceptionsretried by default, but this could be modified by including other codes in thecheckResponsemethod.