-
Notifications
You must be signed in to change notification settings - Fork 189
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
test cache first then download (and don't fail when online doesn't work if it can be resolved to cache) #211
Conversation
@jcompagner you should make sure that your ECA matches the mail address you used in your commit and have added your github user id to your eclipse profile. |
600632e
to
3379b07
Compare
Can you please separate the part about not failing in case of network issue and the part about using cache first into distinct commits/PRs? |
the above patch is just that, that it ignores better http errors and falls back to cached content. It could be a bit nicer IF the p2.index file was also cached. but thats not something tycho can really do anything about. |
Ok. Is the network issue logged as a warning in case of unaccessible remote repo, but file is found in cache? |
yes you see this: the only thing is if i turn of my network i see per repository in my target quite a few lines of those mvn install and the both of them with or without a network... so caching the p2.index would make it a lot faster when network is down and with a lot less logs (only the actual warning of i can't read content.xml.xy ) (which in online mode it would only do) |
if (cacheFile != null) { | ||
return cacheFile; | ||
} | ||
throw new ProvisionException(getFailureStatus(remoteFile)); |
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.
Shouldn't the "throw" here be preserved in case of offline + cache miss? I have the impression modified code would try to download the file in that case.
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.
yes, not sure why i removed that, force pushed a new change
because other metadata (xml, xml.jar or xml.jar.xz) could be found in the cache, it shouldn't break directly with the first (content.xml)
3379b07
to
7a78b7e
Compare
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.
I've added a few cosmetic comments just for the sake of the discussion.
But it's not a blocker for a merge, so let's merged.
@@ -77,14 +77,20 @@ public File createCache(URI repositoryLocation, String prefix, IProgressMonitor | |||
|
|||
@Override | |||
public File createCacheFromFile(URI remoteFile, IProgressMonitor monitor) throws ProvisionException, 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.
Shouldn't the "IOException" be removed as possible "throws" ?
return super.createCacheFromFile(remoteFile, monitor); | ||
try { | ||
return super.createCacheFromFile(remoteFile, monitor); | ||
} catch (IOException e) { |
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.
Could this be merged as IOException | ProvisionException e
?
also make sure that if the download fails that it doesn't fail the
system
This is offline first but it will go online if needed.