-
Notifications
You must be signed in to change notification settings - Fork 194
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
Mirror mojo fails due cache files being too long #4459
Comments
I manage to workaround the issue patching the diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java
index b39b3c5..70ffa9b 100644
--- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java
+++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/transport/SharedHttpCacheStorage.java
@@ -251,17 +251,30 @@
}
updateHeader(response, code);
if (isRedirected(code)) {
- File cachedFile = SharedHttpCacheStorage.this.getCacheEntry(getRedirect(uri), logger)
- .getCacheFile(transportFactory);
- // https://github.com/eclipse-tycho/tycho/issues/2938
- // Redirect may change extension. P2's SimpleMetadataRepositoryFactory relies on
- // accurate file extension to be cached.
- // Copying file to accommodate original request and its file extension.
- // Once https://github.com/eclipse-equinox/p2/issues/355 is fixed, cachedFile
- // may be returned directly without copying.
- response.close(); // early close before doing unrelated file I/O
- FileUtils.copyFile(cachedFile, file);
- return file;
+ try {
+ File cachedFile = SharedHttpCacheStorage.this.getCacheEntry(getRedirect(uri), logger)
+ .getCacheFile(transportFactory);
+ // https://github.com/eclipse-tycho/tycho/issues/2938
+ // Redirect may change extension. P2's SimpleMetadataRepositoryFactory relies on
+ // accurate file extension to be cached.
+ // Copying file to accommodate original request and its file extension.
+ // Once https://github.com/eclipse-equinox/p2/issues/355 is fixed, cachedFile
+ // may be returned directly without copying.
+ response.close(); // early close before doing unrelated file I/O
+ FileUtils.copyFile(cachedFile, file);
+ return file;
+ }catch (IOException e) {
+ logger.warn("Failed to cache file: " + e.getMessage());
+
+ HttpTransport redirectTransport = transportFactory.createTransport(getRedirect(uri));
+ redirectTransport.get(r -> {
+ try (OutputStream os = new BufferedOutputStream(new FileOutputStream(file))) {
+ r.transferTo(os);
+ }
+ return null;
+ });
+ return file;
+ }
}
if (exits) {
FileUtils.forceDelete(file);
|
@rbioteau could you submit this as a pull request? We are facing the same issue (also with jfrog). |
Caching using redirected URLs to cloud/CDN providers as the key is unlikely to function as a cache because the URL is likely to contain a token with a very short lifespan. Repeated requests to the same original URL will yield different redirected URLs and there will never be a cache hit for more than a few minutes. |
@rbioteau we really need this fix, are you able to make a PR? Otherwise I will, but I don't want to steal your work. |
Hi, no I didn't make a PR and I'm not confident enough to suggest one as I don't understanding all the ins and outs of this cache mechanism. Feel free to work on this topic if you feel more confident. |
@TIBCOrkrajews P2 is definitely not designed for usage with dynamic redirects, in this case I would recommend do use some kind of proxy that allows for stable URLs as otherwise you will get poor performance. P2 itself already has the concept of mirror servers for artifacts, so one would better use that then if it is really required. When working on a solution one might distinguish permanent from temporal redirects and do not cache temporal ones, but this will result in the offline functionality to render completely useless, so one should take this into consideration. |
@laeubi the way our Artifactory is set up (thanks to enterprise IT), the redirection isn't even consistent. Sometimes it redirects, but sometimes it doesn't. What I was thinking as a solution is to cache the artifact under the original URL for 302 Found, and cache it under the redirected URL for 301 Moved Permanently (the current behavior for all redirections). This seems to align with the HTTP standard:
Compared to 301 Moved Permanently:
I don't think I would implement this without making some kind of system property to enable it (unless you think it's fine). It seems like a much easier solution than what I'm proposing :) |
Currently the cache just tries to "replay" but in case of redirects we could/should probably improve this yes it was not written with such things in mind and I doubt there is even any tests yet. |
When mirroring a target platform containing the following repository
The mojo fails with the following error:
The text was updated successfully, but these errors were encountered: