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/10/30 01:51:02 UTC
[GitHub] [maven-build-cache-extension] AlexanderAshitkin opened a new pull request, #33: [MBUILDCACHE-32] Do not print exception when probing builds
AlexanderAshitkin opened a new pull request, #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33
… remote repo
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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] alexthomasmc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "alexthomasmc (via GitHub)" <gi...@apache.org>.
alexthomasmc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1176228909
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
Yes that's the scenario I had in mind, i.e. enabling the remote cache is a deliberate act so if it then doesn't work that should be an error not a warning.
In the situation where the user doesn't expect the remote cache to work, e.g. working offline, they should disable it.
--
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] kwin commented on pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#issuecomment-1365656162
Both should end up in separate commits, otherwise finding git commits introducing regressions is much harder than it should be.
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284165766
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@olamy I'm fine with displaying the exception if debug is enabled, but the level of the message is part of the semantic and should not really change depending on which level is activated.
I've used this pattern a log, but I think it should be:
```
if (isDebugEnabled()) {
log.info(xxx, exception);
} else {
log.info(xxx);
}
```
--
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] PayBas commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "PayBas (via GitHub)" <gi...@apache.org>.
PayBas commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1177078475
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
> i.e. enabling the remote cache is a deliberate act so if it then doesn't work that should be an error not a warning.
In the situation where the user doesn't expect the remote cache to work, e.g. working offline, they should disable it.
I don't think that's the intended scenario described by the author.
The remote cache can be working 100% correctly and still not contain the requested items. In fact, this is 100% bound to happen, unless the cache is magically warmed-up beforehand.
1. New project/branch/version/whatever so cache is empty.
2. Build starts
3. Cache extension cannot find cache item of Module X. <-- this is 100% normal and expected behavior and shouldn't throw an error (or even a warming i.m.h.o.), let alone a stack-trace.
4. Module X gets built
5. Module X output gets put into the cache
6. Next build starts, and now it can find cache item of Module X.
Having the logs filled with error/stack-traces for non-issues is counterproductive. So I support any attempt at a more sane output.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284156538
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@gnodet there is `if (LOGGER.isDebugEnabled())` and else `info` and, more important without the exception as the last parameter, which is very verbose. per default, we will be fine.
But could be really more simple to remove this if and using only ` LOGGER.info("Cache item not found: {}", fullUrl);`
--
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] alexthomasmc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "alexthomasmc (via GitHub)" <gi...@apache.org>.
alexthomasmc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1274550737
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
The cache miss exception is caught just before this, so by the time we get here there's a bigger problem. Agree with your policy on this - I think that's the intent of the current logic, it's just the logging that's the issue.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1306832407
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
could we make progress here? It would be fine to have this done and released as it is very verbose for no gain
--
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] kwin commented on pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#issuecomment-1364478639
There are lots of unrelated documentation fixes. Can you separate those into a dedicated PR?
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#issuecomment-1365543617
> There are lots of unrelated documentation fixes. Can you separate those into a dedicated PR?
I would prefer not to do so. The change itself is trivial, the documentation changes are also ok as I understand. Please approve if you don't have objections on the essence of changes.
--
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] alexthomasmc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "alexthomasmc (via GitHub)" <gi...@apache.org>.
alexthomasmc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284163614
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@gnodet Maybe? It's a tradeoff - you want to know how well the cache is working but you don't want to clutter the log (in some of our builds we have nearly 100 modules). Also the "not found" phrase implies to me that something could be wrong but of course a cache miss is completely normal, so perhaps "Item not in cache" would sound more innocent.
A summary at the end of how much the cache was used instead would be very nice...
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284240026
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
Then the individual statements could be at debug level (with exception at trace level eventually), while the summary could be logged at info level.
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284165766
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@olamy I'm fine with displaying the exception if debug is enabled, but the level of the message is part of the semantic and should not really change depending on which level is activated.
I've used this pattern a log, but I think it should be:
```
if (isDebugEnabled()) {
log.info(xxx, exception);
} else {
log.info(xxx);
}
```
It displays more information if the debug logging is enabled, but the message is still `info`, whatever is enabled.
--
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] PayBas commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "PayBas (via GitHub)" <gi...@apache.org>.
PayBas commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1177078475
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
> i.e. enabling the remote cache is a deliberate act so if it then doesn't work that should be an error not a warning.
In the situation where the user doesn't expect the remote cache to work, e.g. working offline, they should disable it.
I don't think that's the intended scenario described by the author.
The remote cache can be working 100% correctly and still not contain the requested items. In fact, this is 100% bound to happen, unless the cache is magically warmed-up beforehand.
1. New project/branch/version/whatever so cache is empty.
2. Build starts
3. Cache extension cannot find cache item of Module X. <-- this is 100% normal and expected behavior and shouldn't throw an error (or even a warming i.m.h.o.), let alone a stack-trace.
4. Module X gets built
5. Module X output gets put into the cache
6. Next build starts, and now it can find cache item of Module X.
Having the logs filled with error/stack-traces for non-issues is counterproductive. So I support any attempt at a more sane output and error-handling.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1175922060
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
> Can I suggest that if we get here it should be at least a warning regardless of whether debug is enabled. I'd go for error() as the remote cache can be easily disabled if it is known that it's not available.
not sure to understand. if the remote cache is disabled. no download will be tried.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1307253655
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@AlexanderAshitkin ping :)
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284061471
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
I think it should be `LOGGER.info`
--
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] ferdnyc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "ferdnyc (via GitHub)" <gi...@apache.org>.
ferdnyc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1261966151
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
@alexthomasmc Is this the remote cache "not working", though, or can this also just represent a cache miss / expired cache?
Cache misses certainly are not errors, heck they're not even problems, they're a standard aspect of how caching works. Data stored in a cache can be wiped out at any moment, for any reason (or even no reason at all). Caches go missing all the time.
When a build system fails to obtain a cached artifact, it simply needs to generate it anew. It can do that, it has the technology.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1181307295
##########
src/test/java/org/apache/maven/buildcache/its/RemoteCacheDavTest.java:
##########
@@ -68,7 +68,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@IntegrationTest("src/test/projects/remote-cache-dav")
-@Testcontainers(disabledWithoutDocker = true)
+@Testcontainers
Review Comment:
why?
at least tests doesn't fail if docker is not available
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1285288318
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
go with that. But as we are in the case of `ResourceDoesNotExistException` not sure why do we need to have a `if` and display the exception.
no if but just:
```
} catch (ResourceDoesNotExistException e) {
LOGGER.info("Cache item not found: {}", fullUrl);
}
```
We are just in the case of a resource not found so no need to print the exception the cause is really clear. I'm not sure to understand of printing a stack even in debug mode. Is there anything more interested in the stack?
Frankly I don't even understand why an exception is used for that. There should be simply a boolean `not found` and not wasting CPU/memory by using an exception (but I know it's a different problem :))
--
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] olamy merged pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy merged PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33
--
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] alexthomasmc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "alexthomasmc (via GitHub)" <gi...@apache.org>.
alexthomasmc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284163614
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
Maybe? It's a tradeoff - you want to know how well the cache is working but you don't want to clutter the log (in some of our builds we have nearly 100 modules). Also the "not found" phrase implies to me that something could be wrong but of course a cache miss is completely normal, so perhaps "Item not in cache" would sound more innocent.
A summary at the end of how much the cache was used instead would be very nice...
--
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 #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#issuecomment-1366070210
> Both should end up in separate commits, otherwise finding git commits introducing regressions is much harder than it should be.
Regression will not happen because of the documentation changes. Code change is isolated and fixes the very clear issue. I'm happy to address any issues with documentation and the code. Let's not overcomplicate this simple 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
[GitHub] [maven-build-cache-extension] gnodet commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1090695427
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +168,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( mavenSession.getRequest().isShowErrors() )
+ {
+ LOGGER.info( "Resource not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Resource not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( mavenSession.getRequest().isShowErrors() )
+ {
+ LOGGER.error( "Cannot download {}", getFullUrl( url ), e );
Review Comment:
Afaik, printing an error usually means the build should fail, or the user should really look at the problem. If the cache isn't available, I don't think this warrant an error, as the build should continue locally instead of downloading the already built artifacts. I would tone it down to `warn` at least.
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +168,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( mavenSession.getRequest().isShowErrors() )
Review Comment:
I think it would be better to use `LOGGER.isDebugEnabled()` rather than the request parameter.
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -64,6 +65,7 @@ public class RemoteCacheRepositoryImpl implements RemoteCacheRepository, Closeab
private final XmlService xmlService;
private final CacheConfig cacheConfig;
+ private final MavenSession mavenSession;
Review Comment:
This should not be needed anymore (see below).
##########
src/site/markdown/getting-started.md:
##########
@@ -17,7 +17,7 @@
## Getting Started
-To on-board incremental Maven you need to complete several steps:
+To on-board incremental Maven need to complete several steps:
Review Comment:
From a dev perspective, having atomic commits is better imho. The doc should be split from the code change are those are completely unrelated.
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
Review Comment:
There are very few differences imho. Do you foresee any change on that part ?
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
Review Comment:
Missing articles. `To get a comprehensive understanding of the build cache machinery`
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
Review Comment:
Missing verb. `Although source code locations are discovered automatically from the project configuration and plugins, there may be edge cases.`
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
+ plugins config, there might be edge cases.
+* Configure remote cache (if remote cache is used)
### Adjusting build cache config
-Having extension run usual command, like `mvn package`. Verify the caching engine is activated:
+Having extension configured run usual command, like `mvn package`. Verify the caching engine is activated:
Review Comment:
Missing verb. Not even sure what is meant here with `Having extension configured run usual command` ...
##########
src/site/markdown/remote-cache.md:
##########
@@ -29,12 +29,11 @@ Before you start, please keep in mind basic principles:
source code file, every dependency and effective pom (including plugin parameters). Every element's hash contributes
to the key. In order to produce the same key there engine must consume exactly the same hashes.
* There is no built-in normalization of line endings in this implementation, file hash calculation is raw bytes
- based. The most obvious implication could be illustrated by a simple Git checkout. By default, git will check out
- source code with CRLF line endings on win and LF on Linux. Because of that builds over same commit on a Linux agent
- and local build on Windows workstation will yield different hashes.
+ based. Common pitfall is a Git checkout with CRLF on Win and LF on Linux. Same commit with different line endings will
Review Comment:
`The same commit`
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
+ plugins config, there might be edge cases.
+* Configure remote cache (if remote cache is used)
### Adjusting build cache config
-Having extension run usual command, like `mvn package`. Verify the caching engine is activated:
+Having extension configured run usual command, like `mvn package`. Verify the caching engine is activated:
-* Check log output - there should be cache related output or initialization error message:
+* Check log output - there should be the cache related output or initialization error message:
```
[INFO] Loading cache configuration from <project dir>/.mvn/maven-build-cache-config.xml
```
* Navigate to your local repo directory - there should be a sibling directory `cache` next to the usual
local `repository`.
* Find `buildinfo.xml` in the cache repository for typical module and review it. Ensure that
- * expected source code files are present in the build info
- * Review all plugings used in the build and add their critical parameters to reconciliation
+ * Expected source code files are present in the build info
Review Comment:
lowercase is preferred I think, as it's the same sentence. Missing article ? `the expected source code`
##########
src/site/markdown/remote-cache.md:
##########
@@ -88,13 +99,12 @@ Allow writes in remote cache add jvm property to designated CI builds.
Run the build, review log and ensure that artifacts are uploaded to remote cache. Now, rerun build and ensure that it
completes almost instantly because it is fully cached.
-### Remote cache relocation to local builds
+### Make remote cache portable
-As practice shows, developers often don't realize that builds they run in local and CI environments are different. So
-straightforward attempt to reuse remote cache in local build usually results in cache misses because of difference in
-plugins, parameters, profiles, environment, etc. In order to reuse results you might need to change poms, cache config,
-CI jobs and the project itself. This part is usually most challenging and time-consuming. Follow steps below to
-iteratively achieve working configuration.
+As practice shows, builds which run on local workstations and CI environments are different. Straightforward attempt to
+reuse cache produced in different environment usually results in cache misses. As is, caches are rarely portable because
+of differences in CI jobs, environment specifics and even project itself. Making cache portable is usually the most
Review Comment:
`even the project`
`Making caches portable`
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
Review Comment:
Missing pronoun.
##########
src/site/markdown/remote-cache.md:
##########
@@ -112,7 +122,7 @@ Copy the link to a `build-cache-report.xml` and provide it to your local build a
mvn verify -Dmaven.build.cache.failFast=true -Dmaven.build.cache.baselineUrl=https://your-cache-url/<...>/915296a3-4596-4eb5-bf37-f6e13ebe087e/build-cache-report.xml
```
-Once discrepancy between remote and local builds detected cache will fail with diagnostic info in
+Once discrepancy between remote and local builds detected cache will fail and spool diagnostic info in
Review Comment:
`Once the discrepancy between remote and local versions is detected, the cache will fail and diagnostic information will be stored in`
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +168,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( mavenSession.getRequest().isShowErrors() )
+ {
+ LOGGER.info( "Resource not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Resource not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( mavenSession.getRequest().isShowErrors() )
+ {
+ LOGGER.error( "Cannot download {}", getFullUrl( url ), e );
Review Comment:
Use `LOGGER.isDebugEnabled()` rather than `mavenSession.getRequest().isShowErrors()`.
##########
src/site/markdown/getting-started.md:
##########
@@ -17,7 +17,7 @@
## Getting Started
-To on-board incremental Maven you need to complete several steps:
+To on-board incremental Maven need to complete several steps:
Review Comment:
There's no pronoun anymore...
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
+ plugins config, there might be edge cases.
+* Configure remote cache (if remote cache is used)
Review Comment:
May be simplify to `Configure remote cache if needed`
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
+ plugins config, there might be edge cases.
+* Configure remote cache (if remote cache is used)
### Adjusting build cache config
-Having extension run usual command, like `mvn package`. Verify the caching engine is activated:
+Having extension configured run usual command, like `mvn package`. Verify the caching engine is activated:
-* Check log output - there should be cache related output or initialization error message:
+* Check log output - there should be the cache related output or initialization error message:
Review Comment:
`a` rather than `the` ?
same for `or an initialization error message` ?
##########
src/site/markdown/remote-cache.md:
##########
@@ -57,19 +56,31 @@ supports http PUT/GET/HEAD operations will suffice (Nginx, Apache or similar). A
change `remote@enabled` to true:
```xml
-<remote enabled="true">
+<!--Default @id is 'cache'-->
+<remote enabled="true" id="my-cache">
<url>http://your-buildcache-url</url>
</remote>
```
If proxy or authentication is required to access remote cache, add server record to settings.xml as described
in [Servers](https://maven.apache.org/settings.html#Servers). The server should be referenced from cache config:
-```
-TBD
+```xml
+
+<servers>
+ <server>
+ <!-- Should match id attribute from the 'remote' tag in cache config or should be 'cache' by default -->
+ <id>my-cache</id>
+ <username>[cache user]</username>
+ <password>[cache password]</password>
+ </server>
+</servers>
```
-Beside the http server, remote cache could be configured using any storage which is supported by [Maven Resolver](https://maven.apache.org/resolver/). That includes a wide set of options, including SSH, FTP and many others through the use of [Maven Wagon](https://maven.apache.org/wagon/). See Wagon documentation for a full list of options and other details.
+Beside the http server, remote cache could be configured using any storage which is supported
Review Comment:
What's the intended meaning of `Beside the http server` here ? Not sure to really grasp it.
##########
src/site/markdown/getting-started.md:
##########
@@ -27,48 +27,55 @@ To on-board incremental Maven you need to complete several steps:
### Declaring build cache extension
```xml
+
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>1.0.0</version>
</extension>
```
-either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`
+either in `pom.xml`'s `<project>/<build>/<extensions>` or in `.mvn/extensions.xml`'s `<extensions>`. Using core
+extension model (`.mvn/extensions.xml` file) is preferable as it allows better access to maven apis and could allow
+more sophisticated optimizations in the future.
### Adding build cache config
Copy template config [`maven-build-cache-config.xml`](../resources/maven-build-cache-config.xml)
to [`.mvn/`](https://maven.apache.org/configure.html) directory of your project.
-To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In typical
-scenario you need to:
+To get overall understanding of build cache machinery, it is recommended to review the config and read comments. In a
+typical scenario need to:
* Exclude unstable, temporary files or environment specific files
-* Add plugins reconciliation rules – add critical plugins parameters to reconciliation
-* Configure precise source code files selectors. Though source code locations discovered automatically from project and plugins config,
- there might be edge cases.
-* Add remote cache location (if remote cache is used)
+* Add critical plugins parameters to runtime reconciliation
+* Configure precise source code files selectors. Though source code locations discovered automatically from project and
+ plugins config, there might be edge cases.
+* Configure remote cache (if remote cache is used)
### Adjusting build cache config
-Having extension run usual command, like `mvn package`. Verify the caching engine is activated:
+Having extension configured run usual command, like `mvn package`. Verify the caching engine is activated:
-* Check log output - there should be cache related output or initialization error message:
+* Check log output - there should be the cache related output or initialization error message:
```
[INFO] Loading cache configuration from <project dir>/.mvn/maven-build-cache-config.xml
```
* Navigate to your local repo directory - there should be a sibling directory `cache` next to the usual
local `repository`.
* Find `buildinfo.xml` in the cache repository for typical module and review it. Ensure that
- * expected source code files are present in the build info
- * Review all plugings used in the build and add their critical parameters to reconciliation
+ * Expected source code files are present in the build info
+ * Review all plugins used in the build and add their critical parameters to reconciliation
Review Comment:
lowercase here too `review all`.
Though the sentence does not make much sense here. One can not ensure to review all plugins. Maybe use a single sentence:
`Ensure that the expected source code files are present in the build info and review all plugins used in the build and add their critical parameters to reconciliation.`
##########
src/site/markdown/remote-cache.md:
##########
@@ -88,13 +99,12 @@ Allow writes in remote cache add jvm property to designated CI builds.
Run the build, review log and ensure that artifacts are uploaded to remote cache. Now, rerun build and ensure that it
completes almost instantly because it is fully cached.
-### Remote cache relocation to local builds
+### Make remote cache portable
-As practice shows, developers often don't realize that builds they run in local and CI environments are different. So
-straightforward attempt to reuse remote cache in local build usually results in cache misses because of difference in
-plugins, parameters, profiles, environment, etc. In order to reuse results you might need to change poms, cache config,
-CI jobs and the project itself. This part is usually most challenging and time-consuming. Follow steps below to
-iteratively achieve working configuration.
+As practice shows, builds which run on local workstations and CI environments are different. Straightforward attempt to
Review Comment:
`Straightforward` -> `A simple`
##########
src/site/markdown/remote-cache.md:
##########
@@ -88,13 +99,12 @@ Allow writes in remote cache add jvm property to designated CI builds.
Run the build, review log and ensure that artifacts are uploaded to remote cache. Now, rerun build and ensure that it
completes almost instantly because it is fully cached.
-### Remote cache relocation to local builds
+### Make remote cache portable
-As practice shows, developers often don't realize that builds they run in local and CI environments are different. So
-straightforward attempt to reuse remote cache in local build usually results in cache misses because of difference in
-plugins, parameters, profiles, environment, etc. In order to reuse results you might need to change poms, cache config,
-CI jobs and the project itself. This part is usually most challenging and time-consuming. Follow steps below to
-iteratively achieve working configuration.
+As practice shows, builds which run on local workstations and CI environments are different. Straightforward attempt to
+reuse cache produced in different environment usually results in cache misses. As is, caches are rarely portable because
Review Comment:
`reuse the cache`
`in a different`
##########
src/site/markdown/remote-cache.md:
##########
@@ -88,13 +99,12 @@ Allow writes in remote cache add jvm property to designated CI builds.
Run the build, review log and ensure that artifacts are uploaded to remote cache. Now, rerun build and ensure that it
completes almost instantly because it is fully cached.
-### Remote cache relocation to local builds
+### Make remote cache portable
-As practice shows, developers often don't realize that builds they run in local and CI environments are different. So
-straightforward attempt to reuse remote cache in local build usually results in cache misses because of difference in
-plugins, parameters, profiles, environment, etc. In order to reuse results you might need to change poms, cache config,
-CI jobs and the project itself. This part is usually most challenging and time-consuming. Follow steps below to
-iteratively achieve working configuration.
+As practice shows, builds which run on local workstations and CI environments are different. Straightforward attempt to
+reuse cache produced in different environment usually results in cache misses. As is, caches are rarely portable because
+of differences in CI jobs, environment specifics and even project itself. Making cache portable is usually the most
+challenging and time-consuming part of setup. Follow steps below to iteratively achieve portable configuration.
Review Comment:
`of the setup`
`Follow the steps`
`achieve a portable`
##########
src/site/markdown/remote-cache.md:
##########
@@ -123,7 +133,7 @@ project's `target/incremental-maven` directory:
Review `buildsdiff.xml` file and eliminate detected discrepancies. You can also diff build-info files directly to get
low level insights. See techniques to configure cache in [How-To](how-to.md) and troubleshooting of typical issues
-in the section below.
+in the section below. Also, it is possible to diff `buildInfo.xml` from remote cache and local workstation directly.
Review Comment:
`from` -> `between`
--
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] alexthomasmc commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "alexthomasmc (via GitHub)" <gi...@apache.org>.
alexthomasmc commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1162909727
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -165,9 +166,28 @@ public Optional<byte[]> getResourceContent( String url ) throws IOException
transporter.get( task );
return Optional.of( task.getDataBytes() );
}
+ catch ( ResourceDoesNotExistException e )
+ {
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cache item not found: {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.info( "Cache item not found: {}", getFullUrl( url ) );
+ }
+ return Optional.empty();
+ }
catch ( Exception e )
{
- LOGGER.info( "Cannot download {}", getFullUrl( url ), e );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Cannot download cache item {}", getFullUrl( url ), e );
+ }
+ else
+ {
+ LOGGER.error( "Cannot download cache item {}: {}", getFullUrl( url ), e );
+ }
Review Comment:
```suggestion
LOGGER.error( "Cannot download cache item: {}", getFullUrl( url ), e );
```
Can I suggest that if we get here it should be at least a warning regardless of whether debug is enabled.
I'd go for error() as the remote cache can be easily disabled if it is known that it's not available.
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284168437
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
@gnodet, why not I'm fine with that. it just looks weird to me :)
--
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] olamy commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #33:
URL: https://github.com/apache/maven-build-cache-extension/pull/33#discussion_r1284171456
##########
src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java:
##########
@@ -144,19 +145,32 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
* @return null or content
*/
@Nonnull
- public Optional<byte[]> getResourceContent(String url) throws IOException {
+ public Optional<byte[]> getResourceContent(String url) {
+ String fullUrl = getFullUrl(url);
try {
- LOGGER.info("Downloading {}", getFullUrl(url));
+ LOGGER.info("Downloading {}", fullUrl);
GetTask task = new GetTask(new URI(url));
transporter.get(task);
return Optional.of(task.getDataBytes());
- } catch (Exception e) {
- LOGGER.info("Cannot download {}", getFullUrl(url), e);
+ } catch (ResourceDoesNotExistException e) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Cache item not found: {}", fullUrl, e);
Review Comment:
> Maybe? It's a tradeoff - you want to know how well the cache is working but you don't want to clutter the log (in some of our builds we have nearly 100 modules). Also the "not found" phrase implies to me that something could be wrong but of course a cache miss is completely normal, so perhaps "Item not in cache" would sound more innocent. A summary at the end of how much the cache was used instead would be very nice...
agree I'm using the cache for a build with 328 modules so the current log is very very verbose when the cache not found :)
--
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