nacx commented on this pull request.
> +
+ public DependencyViolationException() {
+ super();
+ }
+
+ public DependencyViolationException(String arg0, Throwable arg1) {
+ super(arg0, arg1);
+ }
+
+ public DependencyViolationException(String arg0) {
+ super(arg0);
+ }
+
+ public DependencyViolationException(Throwable arg0) {
+ super(arg0);
+ }
Put readable names to all method arguments.
> VPCRequest create(@QueryParam("RegionId") String region, CreateVPCOptions
> vpcOptions);
@Named("vpc:delete")
@POST
@QueryParams(keys = "Action", values = "DeleteVpc")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Actually, we use fallbacks on DELETE operations. If we want to delete a
resource that is no longer there, just don't fail as we are already in the
desired status.
> @@ -138,7 +139,6 @@ VSwitchRequest create(@QueryParam("ZoneId") String zone,
@Named("vswitch:delete")
@POST
@QueryParams(keys = "Action", values = "DeleteVSwitch")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Same here.
> +import org.jclouds.http.HttpCommand;
+import org.jclouds.http.HttpResponse;
+import org.jclouds.http.handlers.RateLimitRetryHandler;
+
+import javax.inject.Inject;
+import javax.inject.Singleton;
+
+import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream;
+
+/**
+ * Handles 403 DependencyViolation.
+ * <p>
+ */
+@Beta
+@Singleton
+public class ECSDependencyViolationRetryHandler extends RateLimitRetryHandler {
I think this is not correct. It is not a rate limit error. You should just
implement a backoff based retry handler.
Also, the `millisToNextAvailableRequest` method reads the `Retry-After` header.
Is that header present at all? How could the backend know when the dependency
would be satisfied?
It makes no sense to me to extend the rate limit handler class. This has to be
changed to a retry handler that implements a backoff based retry.
> + private static final String RETRYABLE_ERROR_CODE = "DependencyViolation";
+
+ @Resource
+ protected Logger logger = Logger.NULL;
+ private final ParseJson<ErrorMessage> parseError;
+
+ @Inject
+ ECSRetryableErrorHandler(ParseJson<ErrorMessage> parseError) {
+ this.parseError = parseError;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse
response) {
+ // Only consider retryable errors and discard rate limit ones
+ if (response.getStatusCode() != 403) {
+ return false;
You should call `super` here. This class just handles 403 errors; otherwise
fallback to the default class that handles retries.
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse
> response) {
+ // Only consider retryable errors and discard rate limit ones
+ if (response.getStatusCode() != 403) {
+ return false;
+ }
+
+ try {
+ // Note that this will consume the response body. At this point,
+ // subsequent retry handlers or error handlers will not be able to
read
+ // again the payload, but that should only be attempted when the
+ // command is not retryable and an exception should be thrown.
+ ErrorMessage error = parseError.apply(response);
+ logger.debug("processing error: %s", error);
+
+ boolean isRetryable = RETRYABLE_ERROR_CODE.equals(error.code());
+ return isRetryable ? super.shouldRetryRequest(command, response) :
false;
This seems to be wrong. If it IS a dependency violation, then impose the
backoff and just retry. If it is not, then we don't handle it, delegate to the
parent class.
> public class BaseECSComputeServiceApiLiveTest extends
> BaseApiLiveTest<ECSComputeServiceApi> {
+ protected static final String TEST_REGION = Regions.EU_CENTRAL_1.getName();
Make this readable form a property that can be passed int he command-line.
--
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/443#pullrequestreview-145559508
