AlexanderAshitkin commented on code in PR #14: URL: https://github.com/apache/maven-build-cache-extension/pull/14#discussion_r874931070
########## src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java: ########## @@ -166,22 +166,22 @@ public CacheResult findCachedBuild( MavenSession session, MavenProject project, ProjectsInputInfo inputInfo = projectInputCalculator.calculateInput( project ); final CacheContext context = new CacheContext( project, inputInfo, session ); - // remote build first - CacheResult result = findCachedBuild( mojoExecutions, context ); Review Comment: > @AlexanderAshitkin Well, that was exactly what I was trying to fix. If there was no remote build cached, but a local build, a lookup did occur... This basically means that local builds are useless if you have a remote cache, because they are never used, which is a bit weird. Not exactly. This is lookup for a local build (which is not from remote cache) ``` // local build first CacheResult result = findLocalBuild( mojoExecutions, context ); ``` This is lookup to find cached build from remote repo (if missed - then lookup remote cache): ``` CacheResult remoteBuild = findCachedBuild( mojoExecutions, context ); ``` Old logic was: 1) Lookup local cache of remote repo 2) If #1 failed lookup remote repo and download 3) If #2 failed (effectively no remote build) - rebuild locally Now logic is: 1) Try to use local build even if build is present in remote repo 2) Try to find locally cached remote build 3) Loockup remote cache The new scheme has minimal performance benefits (both tried to find local build first), but likely will lead to less consistent results and difficult to debug discrepancies in presence of remote cache ########## src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java: ########## @@ -166,22 +166,22 @@ public CacheResult findCachedBuild( MavenSession session, MavenProject project, ProjectsInputInfo inputInfo = projectInputCalculator.calculateInput( project ); final CacheContext context = new CacheContext( project, inputInfo, session ); - // remote build first - CacheResult result = findCachedBuild( mojoExecutions, context ); Review Comment: > @AlexanderAshitkin Well, that was exactly what I was trying to fix. If there was no remote build cached, but a local build, a lookup did occur... This basically means that local builds are useless if you have a remote cache, because they are never used, which is a bit weird. Not exactly. This is lookup for a local build (which is not from remote cache) ``` // local build first CacheResult result = findLocalBuild( mojoExecutions, context ); ``` This is lookup to find cached build from remote repo (if missed - then lookup remote cache): ``` CacheResult remoteBuild = findCachedBuild( mojoExecutions, context ); ``` Old logic was: 1) Lookup local cache of remote repo 2) If \#1 failed lookup remote repo and download 3) If \#2 failed (effectively no remote build) - rebuild locally Now logic is: 1) Try to use local build even if build is present in remote repo 2) Try to find locally cached remote build 3) Loockup remote cache The new scheme has minimal performance benefits (both tried to find local build first), but likely will lead to less consistent results and difficult to debug discrepancies in presence of remote cache -- 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