AlexanderAshitkin commented on code in PR #92:
URL:
https://github.com/apache/maven-build-cache-extension/pull/92#discussion_r1298710444
##########
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:
Hi Kevin. Please elaborate, what are the benefits of implementing this.
Because drawbacks are apparent - cached artifacts not used in the actual build
might fail the build now (exotic classifiers, etc.). If that starts happening,
it is a more serious issue than a delayed error.
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -401,6 +399,26 @@ private Future<File> createDownloadTask(
});
if (!cacheConfig.isLazyRestore()) {
Review Comment:
Looks like a more relevant name for the original intent is `lazyDownload`.
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -129,7 +129,8 @@ public void execute(
boolean restored = result.isSuccess(); // if partially restored
need to save increment
if (restorable) {
restored &= restoreProject(result, mojoExecutions,
mojoExecutionRunner, cacheConfig);
- } else {
+ }
+ if (!restored) {
Review Comment:
Good point. Restoring in the tmp + mv seems like a more robust approach.
--
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
