kbuntrock commented on code in PR #92:
URL:
https://github.com/apache/maven-build-cache-extension/pull/92#discussion_r1301719141
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -401,6 +399,26 @@ private Future<File> createDownloadTask(
});
if (!cacheConfig.isLazyRestore()) {
downloadTask.run();
+ try {
+ downloadTask.get();
Review Comment:
@maximilian-novikov-db : I would rather not since I consider this as part
of the bug I am willing to fix.
Here are some other arguments.
The documentation state the following in the "performance" page :
> Use a lazy restore
>
> By default, the cache tries to restore all artifacts for a project
preemptively. Lazy restore could give significant time by avoiding requesting
and downloading unnecessary artifacts from the cache. It is beneficial when
small changes are a dominating build pattern. Use command line flag:
>
> -Dmaven.build.cache.lazyRestore=true";
>
> In cache corruption situations, the lazy cache cannot support fallback to
normal execution. It will fail instead. To heal the corrupted cache, manually
remove corrupted cache entries or force cache rewrite.
Implied by this piece of documentation : Contrary to the lazy restore, the
pre-emptive default restore option supports fallback. This is actually not the
case for artifacts, the most important build outputs.
Alongside, restoration of generated source and other attached outputs works
as expected : any cache corruption will trigger a regular build.
I have a strong feeling we are just in presence of a simple implementation
oversight I'm fixing here : calling "run" on a FutureTask does the restoration
but do no throw any exception upon failure. It has to be checked via a get
method.
@AlexanderAshitkin
To complete a bit more this question :
> Does it solve any practical issues?
Yes. Actually, if a corrupted/non downloadable artefact is used in a
dependant module while we have not used the "lazy restore" functionality : the
build will fail.
Worst case before this fix : the build fails
Worst case after this fix : we do a regular build.
Seems pretty important that one should be able to choose stability over
rapidity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
