You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by FileIOUtility <no...@github.com> on 2018/07/04 14:25:49 UTC
[jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
do not expect 404 when assets not found,
initially done for Server API only
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/439
-- Commit Summary --
* JCLOUDS-1432 - handle RESOURCE_NOT_FOUND, do not expect 404 when assets not found,
-- File Changes --
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApi.java (8)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/handlers/DimensionDataCloudControlErrorHandler.java (6)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/handlers/ResponseFallbacks.java (46)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApiMockTest.java (22)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/internal/BaseDimensionDataCloudControlMockTest.java (12)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/439.patch
https://github.com/jclouds/jclouds-labs/pull/439.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -56,7 +57,7 @@ public void testDeployServerReturnsUnexpectedError() throws InterruptedException
serverApi().deployServer(ServerApiMockTest.class.getSimpleName(), "imageId", true, networkInfo,
"administratorPassword");
failBecauseExceptionWasNotThrown(ResourceNotFoundException.class);
- } catch (ResourceNotFoundException e) {
+ } catch (HttpResponseException e) {
Then the assertion just before the `catch` needs to be modified, right?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#discussion_r200994940
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FileIOUtility commented on this pull request.
please see my comments and another commit based on your feedback. thanks
> @@ -68,21 +68,19 @@
@GET
@Path("/server")
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class)
For CloudControl list APIs 404 will never be returned, an empty JSON array will be returned instead when there are no results to return.
404 is only used for malformed URIs.
Default behavior does not need to be overridden.
> PagedIterable<Server> listServers();
@Named("server:get")
@GET
@Path("/server/{id}")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+ @Fallback(ResponseFallbacks.NullOnResourceNotFound.class)
Likewise, for get CloudControl get APIs, 404 will never be returned, standard response with responseCode RESOURCE_NOT_FOUND will be returned instead.
> PagedIterable<Server> listServers();
@Named("server:get")
@GET
@Path("/server/{id}")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+ @Fallback(ResponseFallbacks.NullOnResourceNotFound.class)
We already had Fallback here, as inspired by other providers' GET APIs (eg. Google). I think thought that suppressing the exception (ResourceNotFoundException in this case) and returning null is wrong (dangerous).
> @@ -110,7 +108,7 @@ String deployServer(@PayloadParam("name") String name, @PayloadParam("imageId")
@POST
@Path("/deleteServer")
@Produces(MediaType.APPLICATION_JSON)
- @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
+ @Fallback(ResponseFallbacks.VoidOnResourceNotFound.class)
Same for delete APIs (actually all CloudControl APIs) - they never return 404 to signal that resource was not found.
In case of delete APIs, general approach in CloudControl jclouds provider was always to disregard the error if asset was already missing.
> @@ -68,21 +68,19 @@
@GET
@Path("/server")
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class)
EmptyIterableWithMarkerOnNotFoundOr404 only affects responses with 404 code or triggering ResourceNotFoundException, none of which if possible for CloudControl list APIs.
This means that previously the method was failing (as expected) for all 4xx responses, ie. there is no behavior change.
> @@ -121,10 +121,9 @@ public void testListServersWithDatacenterFiltering() throws Exception {
return Uris.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server");
}
- public void testListServers_404() throws Exception {
- server.enqueue(response404());
- assertTrue(serverApi().listServers().concat().isEmpty());
- assertSent(HttpMethod.GET, getListServerUriBuilder().toString());
+ public void testListServers_NoServersFound() {
+ server.enqueue(emptyListResponse("server"));
Yes, this test is testing a 200 response. This was intended as an empty list response test.
404 is not possible, as previously mentioned, hence the rewrite.
For error code 400 - there is already testDeployServerReturnsUnexpectedError
(Actually it is a preexisting bug in DimensionDataCloudControlErrorHandler that responseCode = UNEXPECTED_ERROR results in ResourceNotFoundException)
Maybe we need 400 test for list and get Server. I can add these, but I am not sure if it belongs here.
> + return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
+
+ public static final class VoidOnResourceNotFound implements Fallback<Void> {
+ public Void createOrPropagate(Throwable t) {
+ if (t instanceof ResourceNotFoundException) {
+ return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
I see now that NullOnNotFoundOr404 / VoidOnNotFoundOr404 might do the job here (they also trigger on ResourceNotFoundException). Let me check, maybe I can revert to using them.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-134401406
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
It's because the default build config. No need to close the PR. I'll bump the source and target versions in the build config and it should be fine.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403549347
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks @FileIOUtility!
> @@ -68,21 +68,19 @@
@GET
@Path("/server")
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class)
This is a behavior change since this method will now fail upon 4xx responses, when previously it just returned an empty list. Consider adding the corresponding fallback and keep returning empty lists.
> PaginatedCollection<Server> listServers(DatacenterIdListFilters datacenterIdListFilters);
@Named("server:list")
@GET
@Path("/server")
@Transform(ParseServers.ToPagedIterable.class)
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
Same here
> @@ -66,9 +66,9 @@ public void handleError(HttpCommand command, HttpResponse response) {
exception = new AuthorizationException(message, exception);
break;
case 404:
- if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
- exception = new ResourceNotFoundException(message, exception);
- }
+ // CloudControl uses error code 400 with RESOURCE_NOT_FOUND to report missing assets
+ // 404 means malformed URI only
+ exception = new IllegalStateException(message, exception);
A malformed URL in a request is better represented as an `IllegalArgumentException`. Illegal sates are more for 409 (Conflict) errors.
> + return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
+
+ public static final class VoidOnResourceNotFound implements Fallback<Void> {
+ public Void createOrPropagate(Throwable t) {
+ if (t instanceof ResourceNotFoundException) {
+ return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
You'd better copy the [default behavior](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/Fallbacks.java#L132) but changing the code to 400, to make sure you just capture the right responses.
> @@ -121,10 +121,9 @@ public void testListServersWithDatacenterFiltering() throws Exception {
return Uris.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server");
}
- public void testListServers_404() throws Exception {
- server.enqueue(response404());
- assertTrue(serverApi().listServers().concat().isEmpty());
- assertSent(HttpMethod.GET, getListServerUriBuilder().toString());
+ public void testListServers_NoServersFound() {
+ server.enqueue(emptyListResponse("server"));
This test does not exercise the fallback, as it returns a 200 response. Change it to return a 400 error code (you'll need the fallbacks for the empty list mentioned previously).
> @@ -135,10 +134,10 @@ public void testGetServer() throws Exception {
assertNotNull(found.guest().vmTools());
}
- public void testGetServer_404() throws Exception {
- server.enqueue(response404());
- serverApi().getServer("12345");
- assertSent(GET, "/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server/12345");
+ public void testGetServer_NotFound() {
+ server.enqueue(responseResourceNotFound());
+ Server foundServer = serverApi().getServer("12345");
+ assertNull(foundServer, "should return null when Server was not found");
Same here. Get back the test that exercises the 400 fallback.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-134404077
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
Closed #439.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#event-1738328709
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FYI: build was aborted during "jclouds jdbc core" phase.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403468803
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
@FileIOUtility pushed 1 commit.
77e3a33 JCLOUDS-1432 - review - replace IllegalStateException
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439/files/10d9e4df56e300cab58eebfdcdea86ae28e8fee3..77e3a337dcb5b53c1d60f2cf9ebf3dea9a451653
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FileIOUtility commented on this pull request.
> } else if (message.contains("NAME_NOT_UNIQUE")) {
exception = new ResourceAlreadyExistsException(message, exception);
+ } else if (message.contains("UNEXPECTED_ERROR")
+ || message.contains("RETRYABLE_SYSTEM_ERROR")
+ || message.contains("SYSTEM_ERROR")) {
+ break;
+ } else {
+ exception = new IllegalArgumentException(message, exception);
There are too many responseCode value to list here. Rather than that, it is better to default to IllegalArgumentException
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-135073335
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
FTR: https://github.com/jclouds/jclouds/pull/1224 should fix this build once merged.
There is one checkstyle violation in this branch though:
```
[WARNING] src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApiMockTest.java[31:8] (imports) UnusedImports: Unused import - org.jclouds.rest.ResourceNotFoundException.
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403553840
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
@FileIOUtility pushed 1 commit.
5d1305b JCLOUD-51 - review - revisit error propagation,
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439/files/77e3a337dcb5b53c1d60f2cf9ebf3dea9a451653..5d1305b5ce3af48226b700595e418a8355a926cb
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
Is it OK to squash these 3 commits before you merge?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402666510
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
Closed #439.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#event-1723645736
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Trevor Flanagan <no...@github.com>.
This is @FileIOUtility first PR for Apache jclouds but he has been working on several aspects of the project for a while now.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402496147
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
@FileIOUtility pushed 1 commit.
56fb4bc JCLOUDS-1432 - removed unused import
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439/files/6e076b3f0c66d9556cd5a3241f0036d671b06b29..56fb4bcd1a8ebea191cfa6fcab443ffdd196d8f0
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
build failed again after I committed the suggested change that needs at least Java 7 to work:
"Undefined reference: void java.util.ConcurrentModificationException.<init>(String, Throwable)"
https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/1994/org.apache.jclouds.labs$dimensiondata-cloudcontrol/consoleFull
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403548925
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
@FileIOUtility pushed 1 commit.
10d9e4d JCLOUDS-1432 - review - revert to using NullOnNotFoundOr404/voidOnNotFoundOr404,
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439/files/fe612335e81ecf414a468b58e2703a911e8b4249..10d9e4df56e300cab58eebfdcdea86ae28e8fee3
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
I think this is correct.
I will add this as 4th commit (before squashing).
RESOURCE_BUSY - Another operation is in progress on an asset. Could alternatively be mapped to ConcurrentModificationException. But it is not on the list of exceptions allowed to propagate.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402674579
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
rebuild please
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403472574
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
>EDIT: in Java language level 6, ConcurrentModificationException cannot take cause exception, so after all, I don't want to use it.
jclouds needs at least Java 7 to work. We've been trying to keep backward compatibility in our builds, but we are about to move to Java 8 in the next major release. I don't think it makes sense to keep worrying about Java 6 compat.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-403338895
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
Reopened #439.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#event-1723645985
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
Yes, I will add it to Throwables2.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402714108
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
merged to master as [2d9cb407](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/2d9cb407). Thanks @FileIOUtility!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-405668430
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -56,7 +57,7 @@ public void testDeployServerReturnsUnexpectedError() throws InterruptedException
serverApi().deployServer(ServerApiMockTest.class.getSimpleName(), "imageId", true, networkInfo,
"administratorPassword");
failBecauseExceptionWasNotThrown(ResourceNotFoundException.class);
- } catch (ResourceNotFoundException e) {
+ } catch (HttpResponseException e) {
Why does this need to be changed?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-135258955
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FileIOUtility commented on this pull request.
revisited error propagation as suggested
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-135072543
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FileIOUtility commented on this pull request.
> @@ -56,7 +57,7 @@ public void testDeployServerReturnsUnexpectedError() throws InterruptedException
serverApi().deployServer(ServerApiMockTest.class.getSimpleName(), "imageId", true, networkInfo,
"administratorPassword");
failBecauseExceptionWasNotThrown(ResourceNotFoundException.class);
- } catch (ResourceNotFoundException e) {
+ } catch (HttpResponseException e) {
On UNEXPECTED_ERROR, deployServer API will now throw HttpResponseException.
ResourceNotFound did not make any sense in that case.
HttpResponseException means that we don't know that is going on - error was unexpected.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#discussion_r200956678
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
FileIOUtility commented on this pull request.
replaced IllegalStateException with IllegalArgumentException
> @@ -66,9 +66,9 @@ public void handleError(HttpCommand command, HttpResponse response) {
exception = new AuthorizationException(message, exception);
break;
case 404:
- if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
- exception = new ResourceNotFoundException(message, exception);
- }
+ // CloudControl uses error code 400 with RESOURCE_NOT_FOUND to report missing assets
+ // 404 means malformed URI only
+ exception = new IllegalStateException(message, exception);
thanks, done
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-134439192
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
And build is back to green! 😄 @FileIOUtility mind squashing the commits so I can cleanly merge the PR?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-404567165
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
No need for a new PR. Just squash and push+force here.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-404577707
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
>RESOURCE_BUSY - Another operation is in progress on an asset. Could alternatively be mapped to ConcurrentModificationException. But it is not on the list of exceptions allowed to propagate.
Looks like a good candidate to be added to the list. Wanna add it too and use that exception here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402676461
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
Thanks, @FileIOUtility! LGTM
From the discussion about response codes, I've revisited the error handler and there are some errors we could consider propagating better:
```java
case 400:
if (message.contains("RESOURCE_NOT_FOUND")
|| message.contains("OPERATION_NOT_SUPPORTED")) {
exception = new ResourceNotFoundException(message, exception);
} else if (message.contains("INVALID_INPUT_DATA")
|| message.contains("ORGANIZATION_NOT_VERIFIED")
|| (message.contains("SYSTEM_ERROR")
&& !message.contains("RETRYABLE_SYSTEM_ERROR"))
|| message.contains("CPU_SPEED_NOT_AVAILABLE")
|| message.contains("CONFIGURATION_NOT_SUPPORTED")) {
exception = new IllegalStateException(message, exception);
} else if (message.contains("RESOURCE_BUSY")
|| message.contains("UNEXPECTED_ERROR")) {
exception = new ResourceNotFoundException(message, exception);
} else if (message.contains("NAME_NOT_UNIQUE")) {
exception = new ResourceAlreadyExistsException(message, exception);
}
break;
```
I'd say it makes more sense to remap the following error codes:
* `RESOURCE_BUSY` -> `IllegalStateException`
* `UNEXPECTED_ERROR`, `SYSTEM_ERROR` and `RETRYABLE_SYSTEM_ERROR` -> We don't know what's going on. Let's keep the default `HttpResponseException` instead of remapping them.
* `OPERATION_NOT_SUPPORTED` -> An `UnsupportedOperationException` looks better?
* `INVALID_INPUT_DATA`, `CPU_SPEED_NOT_AVAILABLE`, `CONFIGURATION_NOT_SUPPORTED` -> `IllegalArgumentException`
Since this PR is about fixinf the handling of an error code, I think we could add these changes in this PR. WDYT?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-402667485
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
Great to see that build, so far I had build failures resulting from different code quality validations applied for JDK7. I will squash and create a new PR.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-404570990
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by FileIOUtility <no...@github.com>.
@FileIOUtility pushed 2 commits.
b13d0f8 JCLOUDS-1432 - review - use JDK7 and ConcurrentModificationException
6e076b3 JCLOUDS-1432 - review - fix test failure message
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439/files/5d1305b5ce3af48226b700595e418a8355a926cb..6e076b3f0c66d9556cd5a3241f0036d671b06b29
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
> @@ -121,10 +121,9 @@ public void testListServersWithDatacenterFiltering() throws Exception {
return Uris.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server");
}
- public void testListServers_404() throws Exception {
- server.enqueue(response404());
- assertTrue(serverApi().listServers().concat().isEmpty());
- assertSent(HttpMethod.GET, getListServerUriBuilder().toString());
+ public void testListServers_NoServersFound() {
+ server.enqueue(emptyListResponse("server"));
It is OK now that there is no fallback defined for those methods.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#pullrequestreview-134574406
Re: [jclouds/jclouds-labs] JCLOUDS-1432 - handle RESOURCE_NOT_FOUND,
(#439)
Posted by Ignasi Barrera <no...@github.com>.
Everything should be in place now in jclouds-core: rebuild please
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/439#issuecomment-404305499