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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
