Skip to content
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

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

jcompagner
Copy link
Contributor

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.

@laeubi
Copy link
Member

laeubi commented Aug 3, 2021

@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.

@jcompagner jcompagner force-pushed the offline_first branch 2 times, most recently from 600632e to 3379b07 Compare August 3, 2021 08:19
@mickaelistria
Copy link
Contributor

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?
While the first part can be straightforward to merge, the second one would require more work to behave as expected (eg support for -U and regular re-check like Maven snapshots)

@jcompagner
Copy link
Contributor Author

the above patch is just that, that it ignores better http errors and falls back to cached content.
I don't care to much about the offline first part (because that could be more problematic, the cache manager really should understand when to or not to cache based on a hash created from the target file, else updates could not be found if we never do that)
But i don't care to much about that, if tycho at least uses the cached stuff and just goes on and tries to build if there are network errors then thats fine for me (and thats the current pr)

It could be a bit nicer IF the p2.index file was also cached. but thats not something tycho can really do anything about.
For that we need to change p2 code itself.

@mickaelistria
Copy link
Contributor

Ok. Is the network issue logged as a warning in case of unaccessible remote repo, but file is found in cache?

@jcompagner
Copy link
Contributor Author

yes you see this:

https://github.com/eclipse/tycho/blob/master/tycho-bundles/org.eclipse.tycho.p2.resolver.impl/src/main/java/org/eclipse/tycho/p2/remote/RemoteRepositoryCacheManager.java#L106

the only thing is if i turn of my network i see per repository in my target quite a few lines of those
that is because the p2.index is not cached by the p2 code and because of that the suffix list is always something like 6 (so it tries 6 things per "content.xxxx" and "artifacts.xxx" which doesn't happen if the index page was there because the index page right away says use only this suffix
That did cost me quite some time to understand why the result was so different when i hit

mvn install
mvn install -o

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));
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

@mickaelistria mickaelistria left a 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 {
Copy link
Contributor

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) {
Copy link
Contributor

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 ?

@mickaelistria mickaelistria merged commit c19c5e1 into eclipse-tycho:master Aug 6, 2021
@laeubi laeubi added this to the 2.5 milestone Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants