Looks good to me. -- Igor
> On Mar 8, 2018, at 4:06 PM, Erik Joelsson <erik.joels...@oracle.com> wrote: > > On 2018-03-08 15:24, Igor Ignatyev wrote: >> Hi Erik, > Thanks for looking at this! >> to avoid incompatibility, you could have just made ArtifactResolverException >> a subclass of java.io.FileNotFoundException. > This is correct, but I don't think the exception should be of type > FileNotFoundException. IMO, this is something different and trying to > re-purpose an existing exception type is rarely a good idea. >> it seems you forgot to add ArtifactResolverException.java file to the repo. > Doh! Added. >> a minor nit: in JibArtifactManager::newInstance, you pass "Could not resolve >> " + JIB_SERVICE_FACTORY to ClassNotFoundException constructor. by the >> convention, the message in CNFE is the classname. > Right, good point, fixed. > > New Webrev: http://cr.openjdk.java.net/~erikj/8199352/webrev.02/ > > /Erik >> -- Igor >> >>> On Mar 8, 2018, at 2:08 PM, Erik Joelsson <erik.joels...@oracle.com> wrote: >>> >>> The Jib artifact resolver is not very good at telling us why things go >>> wrong. The reason is that it swallows exceptions. This patch changes the >>> API from throwing a FileNotFoundException, which I don't really think fits >>> correctly in all cases, to a new API specific exception. >>> >>> I have greped for all uses of this API in the tests and changed the >>> exception type caught at the caller location. I verified that I didn't >>> break anything by compiling all the affected test classes and by running >>> some of them for a bit. >>> >>> With these changes it should be easier to diagnose problems with resolving >>> artifacts in the future. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199352 >>> >>> Webrev: http://cr.openjdk.java.net/~erikj/8199352/webrev.01/index.html >>> >>> /Erik >>> >
