You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/05/16 19:00:08 UTC
[GitHub] [maven-build-cache-extension] gnodet opened a new pull request, #14: [MBUILDCACHE-20] Use local cache before remote cache by default
gnodet opened a new pull request, #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14
@maximilian-novikov @maximilian-novikov-db could you have a look at this one ?
I don't really understand why the remote cache would have to be checked before the local one...
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
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 already downloaded build from remote cache. If not downloaded - 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) - try use local build
Now logic is:
1) Try to use local build even if reference build is present in remote repo
2) Try to find locally cached remote(reference) build
3) Lookup remote cache if 2 failed
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.
Performance gain happens in the only scenario when build is purely - local - never was installed to remote repo.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] gnodet merged pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
gnodet merged PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] gnodet commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#discussion_r874823004
##########
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:
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...
##########
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 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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] gnodet commented on pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#issuecomment-1128477176
> > @maximilian-novikov @maximilian-novikov-db could you have a look at this one ? I don't really understand why the remote cache would have to be checked before the local one...
>
> that's the good question, probably that's the legacy logic, which doesn't make sense after @AlexanderAshitkin added 'proper' plugins input reconciliation
>
> i would just fix this, without introducing a new flag
Ok, I'll simplify the PR then.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#discussion_r874814667
##########
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:
@gnodet @maximilian-novikov-db remote honestly this change brings regression.
There are 2 local caches:
1) local cache of remote repository
2) cache of local builds
Previously, if remote build was present in local cache it was always used - there was no lookup. Now local build used with priority which could lead to difficult to debug issues.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] gnodet commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#discussion_r874823004
##########
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#discussion_r874814667
##########
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:
@gnodet @maximilian-novikov-db remote honestly this change brings regression.
There are 2 local caches:
1) local cache of remote repository
2) cache of local builds
Previously, if remote build was present in local cache it was always used - there was no lookup. Now local build used with priority which could lead to difficult to debug issues. And performance benefits are minimal
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
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 a 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) - try use local build
Now logic is:
1) Try to use local build even if reference build is present in remote repo
2) Try to find locally cached remote(reference) build
3) Lookup remote cache if 2 failed
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] maximilian-novikov-db commented on pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
maximilian-novikov-db commented on PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#issuecomment-1128471321
> @maximilian-novikov @maximilian-novikov-db could you have a look at this one ? I don't really understand why the remote cache would have to be checked before the local one...
that's the good question, probably that's the legacy logic, which doesn't make sense after @AlexanderAshitkin added 'proper' plugins input reconciliation
i would just fix this, without introducing a new flag
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
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) - try use local build
Now logic is:
1) Try to use local build even if reference build is present in remote repo
2) Try to find locally cached remote(reference) build
3) Lookup remote cache if 2 failed
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#issuecomment-1128838217
> > > @maximilian-novikov @maximilian-novikov-db could you have a look at this one ? I don't really understand why the remote cache would have to be checked before the local one...
>
> >
>
> > that's the good question, probably that's the legacy logic, which doesn't make sense after @AlexanderAshitkin added 'proper' plugins input reconciliation
>
> >
>
> > i would just fix this, without introducing a new flag
>
>
>
> Ok, I'll simplify the PR then.
Hi
The idea was to use the same artifact across all the cache consumers. By looking in the remote first it's possible to avoid using locally built artifacts and cached binaries should become eventually consistent across all nodes, even if remote cache was overridden. I cannot recall other implications except that, but discrepancies theoretically possible if binaries compared directly(snapshots?). But definitely local first lookup will work in majority of cases
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
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) - try use local build
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-build-cache-extension] gnodet commented on pull request #14: [MBUILDCACHE-20] Use local cache before remote cache by default
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #14:
URL: https://github.com/apache/maven-build-cache-extension/pull/14#issuecomment-1129321988
I reverted the change.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org