Improve errors handling while downloading p2 artifacts from repository #4276
Conversation
|
Follow-up to the discussion in eclipse-windowbuilder/windowbuilder#882. We still get build errors due to a bad mirror, which look something like: I sadly don't have a good way to test this issue locally, as the mirror I usually get is up-to-date. |
ptziegler
force-pushed
the
retry-handling
branch
from
45fa357
to
246a2a3
Compare
| @@ -47,6 +47,8 @@ | |||
| @Component(role = P2RepositoryManager.class) | |||
| public class P2RepositoryManager { | |||
|
|
|||
| private static final int RETRY_ATTEMPTS = 3; | |||
There was a problem hiding this comment.
I'm not overly happy with this magic number. Perhaps there's a better way to define the number of retries? Perhaps it should even be configurable?
There was a problem hiding this comment.
You can define a property and then use MavenPropertyHelper (don't forget to document the property here)
laeubi
added
the
backport-to-tycho-4.0.x
Yes this is very hard (and annoying) to test... Basically if one wants to go that way look at
That way P2 should see one failing and one succeeding request and in the end the download should work + maybe seeing a message in the logs. |
ptziegler
force-pushed
the
retry-handling
branch
from
246a2a3
to
642c1c1
Compare
I fear 4. is not really possible. I've created a test which -in principle- works, except that I can't reliably pick the mirror over the source repository. The problem is this method here, which effectively picks a random mirror: private MirrorInfo selectMirror(IProgressMonitor monitor) {
initMirrors(monitor);
final int mirrorCount;
if (mirrors == null || (mirrorCount = mirrors.length) == 0)
return null;
MirrorInfo selected;
if (mirrorCount == 1)
selected = mirrors[0];
else {
Arrays.sort(mirrors, getComparator());
for (;;) {
//this is a function that randomly selects a mirror based on a logarithmic
//distribution. Mirror 0 has a 1/2 chance of being selected, mirror 1 has a 1/4 chance,
// mirror 2 has a 1/8 chance, etc. This introduces some variation in the mirror
//selection, while still heavily favoring better mirrors
//the algorithm computes the most significant digit in a binary number by computing the base 2 logarithm
//if the first digit is most significant, mirror 0 is selected, if the second is most significant, mirror 1 is selected, etc
int highestMirror = min(15, mirrorCount);
int result = (int) (Math.log(random.nextInt(1 << highestMirror) + 1) / LOG2);
if (result >= highestMirror || result < 0)
result = highestMirror - 1;
int mirrorIndex = highestMirror - 1 - result;
selected = mirrors[mirrorIndex];
// Only choose a mirror from the best 50% of the top 15 of all mirrors
if (mirrorIndex <= (mirrorCount * 0.5d))
// This is good enough
break;
}
}
//for now, don't tolerate mirrors with multiple failures
if (selected.failureCount > 1)
return null;
return selected;
}But if you run the test often enough, you eventually see the following warning, which is exactly what I wanted to add: |
ptziegler
force-pushed
the
retry-handling
branch
from
642c1c1
to
a747bda
Compare
You can add a new method similar to Another option would be to allow registering a Filter, as filters can intercept the calls to servlets and e.g. return 404 for the first request. |
In case an artifact couldn't be downloaded due to e.g. a bad mirror, p2 returns an RETRY error code. This code is currently ignored and Tycho simply fails with an I/O exception. Instead, Tycho attempts to download the artifact up to three times (assuming that this error code was returned), before failing. This value can be configured using the 'eclipse.p2.maxDownloadAttempts' system property.
ptziegler
force-pushed
the
retry-handling
branch
from
a747bda
to
b615532
Compare
Another option would be to allow registering a Filter, as filters can intercept the calls to servlets and e.g. return 404 for the first request. In principle,yes. Though Tycho does a lot of caching, which quickly led to situations where only the initial request was sent. The only way to reliably get around this problem was by using |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
|
I've just tried out the latest nightly build and this fix already prevented the first crash! 😄
@laeubi Thanks for pointing me in the right direction, especially with the unit test! |
|
Thanks both of you! |
In case an artifact couldn't be downloaded due to e.g. a bad mirror, Equinox returns an RETRY error code. This code is currently ignored and Tycho simply fails with an I/O exception.
Instead, Tycho attempts to download the artifact up to three times (assuming that this error code was returned), before failing.