[GitHub] [maven-build-cache-extension] PayBas commented on a diff in pull request #33: [MBUILDCACHE-32] Do not print exception when probing builds

["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