Skip to content

Commit

Permalink
Marked exceptions for create and deletes in DNS as non-idempotent. (#883
Browse files Browse the repository at this point in the history 
)

Add the notion of "rejected" to BaseServiceException to indicate that the service rejected the request and it should be safe to retry it even if request is not idempotent.
  • Loading branch information
@mderka @aozarov
mderka authored and aozarov committed Apr 14, 2016
1 parent 13ab36f commit fd740ea
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 47 deletions.
60 changes: 32 additions & 28 deletions gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,16 @@ protected static final class Error implements Serializable {

private final Integer code;
private final String reason;
private final boolean rejected;

public Error(Integer code, String reason) {
this(code, reason, false);
}

public Error(Integer code, String reason, boolean rejected) {
this.code = code;
this.reason = reason;
this.rejected = rejected;
}

/**
Expand All @@ -61,18 +67,27 @@ public Integer code() {
return code;
}

/**
* Returns true if the error indicates that the API call was certainly not accepted by the
* server. For instance, if the server returns a rate limit exceeded error, it certainly did not
* process the request and this method will return {@code true}.
*/
public boolean rejected() {
return rejected;
}

/**
* Returns the reason that caused the exception.
*/
public String reason() {
return reason;
}

boolean isRetryable(Set<Error> retryableErrors) {
boolean isRetryable(boolean idempotent, Set<Error> retryableErrors) {
for (Error retryableError : retryableErrors) {
if ((retryableError.code() == null || retryableError.code().equals(this.code()))
&& (retryableError.reason() == null || retryableError.reason().equals(this.reason()))) {
return true;
return idempotent || retryableError.rejected();
}
}
return false;
Expand All @@ -95,12 +110,14 @@ public BaseServiceException(IOException exception, boolean idempotent) {
String reason = null;
String location = null;
String debugInfo = null;
Boolean retryable = null;
if (exception instanceof GoogleJsonResponseException) {
GoogleJsonError jsonError = ((GoogleJsonResponseException) exception).getDetails();
if (jsonError != null) {
Error error = error(jsonError);
Error error = new Error(jsonError.getCode(), reason(jsonError));
code = error.code;
reason = error.reason;
retryable = isRetryable(idempotent, error);
if (reason != null) {
GoogleJsonError.ErrorInfo errorInfo = jsonError.getErrors().get(0);
location = errorInfo.getLocation();
Expand All @@ -110,22 +127,16 @@ public BaseServiceException(IOException exception, boolean idempotent) {
code = ((GoogleJsonResponseException) exception).getStatusCode();
}
}
this.retryable = MoreObjects.firstNonNull(retryable, isRetryable(idempotent, exception));
this.code = code;
this.retryable = idempotent && isRetryable(exception);
this.reason = reason;
this.idempotent = idempotent;
this.location = location;
this.debugInfo = debugInfo;
}

public BaseServiceException(GoogleJsonError error, boolean idempotent) {
super(error.getMessage());
this.code = error.getCode();
this.reason = reason(error);
this.idempotent = idempotent;
this.retryable = idempotent && isRetryable(error);
this.location = null;
this.debugInfo = null;
this(error.getCode(), error.getMessage(), reason(error), idempotent);
}

public BaseServiceException(int code, String message, String reason, boolean idempotent) {
Expand All @@ -138,7 +149,7 @@ public BaseServiceException(int code, String message, String reason, boolean ide
this.code = code;
this.reason = reason;
this.idempotent = idempotent;
this.retryable = idempotent && new Error(code, reason).isRetryable(retryableErrors());
this.retryable = isRetryable(idempotent, new Error(code, reason));
this.location = null;
this.debugInfo = null;
}
Expand All @@ -147,15 +158,12 @@ protected Set<Error> retryableErrors() {
return Collections.emptySet();
}

protected boolean isRetryable(GoogleJsonError error) {
return error != null && error(error).isRetryable(retryableErrors());
protected boolean isRetryable(boolean idempotent, Error error) {
return error.isRetryable(idempotent, retryableErrors());
}

protected boolean isRetryable(IOException exception) {
if (exception instanceof GoogleJsonResponseException) {
return isRetryable(((GoogleJsonResponseException) exception).getDetails());
}
return exception instanceof SocketTimeoutException;
protected boolean isRetryable(boolean idempotent, IOException exception) {
return idempotent && exception instanceof SocketTimeoutException;
}

/**
Expand Down Expand Up @@ -187,8 +195,8 @@ public boolean idempotent() {
}

/**
* Returns the service location where the error causing the exception occurred. Returns
* {@code null} if not set.
* Returns the service location where the error causing the exception occurred. Returns {@code
* null} if not available.
*/
public String location() {
return location;
Expand Down Expand Up @@ -223,18 +231,14 @@ public int hashCode() {
debugInfo);
}

protected static String reason(GoogleJsonError error) {
if (error.getErrors() != null && !error.getErrors().isEmpty()) {
private static String reason(GoogleJsonError error) {
if (error.getErrors() != null && !error.getErrors().isEmpty()) {
return error.getErrors().get(0).getReason();
}
return null;
}

protected static Error error(GoogleJsonError error) {
return new Error(error.getCode(), reason(error));
}

protected static String message(IOException exception) {
private static String message(IOException exception) {
if (exception instanceof GoogleJsonResponseException) {
GoogleJsonError details = ((GoogleJsonResponseException) exception).getDetails();
if (details != null) {
Expand Down
16 changes: 8 additions & 8 deletions gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,16 @@ public class DnsException extends BaseServiceException {

// see: https://cloud.google.com/dns/troubleshooting
private static final Set<Error> RETRYABLE_ERRORS = ImmutableSet.of(
new Error(429, null),
new Error(500, null),
new Error(502, null),
new Error(503, null),
new Error(null, "userRateLimitExceeded"),
new Error(null, "rateLimitExceeded"));
new Error(429, null, true),
new Error(500, null, false),
new Error(502, null, false),
new Error(503, null, false),
new Error(null, "userRateLimitExceeded", true),
new Error(null, "rateLimitExceeded", true));
private static final long serialVersionUID = 490302380416260252L;

public DnsException(IOException exception) {
super(exception, true);
public DnsException(IOException exception, boolean idempotent) {
super(exception, idempotent);
}

private DnsException(int code, String message) {
Expand Down
23 changes: 12 additions & 11 deletions gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/DefaultDnsRpc.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ public class DefaultDnsRpc implements DnsRpc {
private final Dns dns;
private final DnsOptions options;

private static DnsException translate(IOException exception) {
return new DnsException(exception);
private static DnsException translate(IOException exception, boolean idempotent) {
return new DnsException(exception, idempotent);
}

/**
Expand All @@ -61,7 +61,8 @@ public ManagedZone create(ManagedZone zone, Map<Option, ?> options) throws DnsEx
.setFields(FIELDS.getString(options))
.execute();
} catch (IOException ex) {
throw translate(ex);
// todo this can cause misleading report of a failure, intended to be fixed within #924
throw translate(ex, true);
}
}

Expand All @@ -73,7 +74,7 @@ public ManagedZone getZone(String zoneName, Map<Option, ?> options) throws DnsEx
.setFields(FIELDS.getString(options))
.execute();
} catch (IOException ex) {
DnsException serviceException = translate(ex);
DnsException serviceException = translate(ex, true);
if (serviceException.code() == HTTP_NOT_FOUND) {
return null;
}
Expand All @@ -93,7 +94,7 @@ public ListResult<ManagedZone> listZones(Map<Option, ?> options) throws DnsExcep
.execute();
return of(zoneList.getNextPageToken(), zoneList.getManagedZones());
} catch (IOException ex) {
throw translate(ex);
throw translate(ex, true);
}
}

Expand All @@ -103,7 +104,7 @@ public boolean deleteZone(String zoneName) throws DnsException {
dns.managedZones().delete(this.options.projectId(), zoneName).execute();
return true;
} catch (IOException ex) {
DnsException serviceException = translate(ex);
DnsException serviceException = translate(ex, false);
if (serviceException.code() == HTTP_NOT_FOUND) {
return false;
}
Expand All @@ -126,7 +127,7 @@ public ListResult<ResourceRecordSet> listRecordSets(String zoneName, Map<Option,
.execute();
return of(response.getNextPageToken(), response.getRrsets());
} catch (IOException ex) {
throw translate(ex);
throw translate(ex, true);
}
}

Expand All @@ -136,7 +137,7 @@ public Project getProject(Map<Option, ?> options) throws DnsException {
return dns.projects().get(this.options.projectId())
.setFields(FIELDS.getString(options)).execute();
} catch (IOException ex) {
throw translate(ex);
throw translate(ex, true);
}
}

Expand All @@ -148,7 +149,7 @@ public Change applyChangeRequest(String zoneName, Change changeRequest, Map<Opti
.setFields(FIELDS.getString(options))
.execute();
} catch (IOException ex) {
throw translate(ex);
throw translate(ex, false);
}
}

Expand All @@ -160,7 +161,7 @@ public Change getChangeRequest(String zoneName, String changeRequestId, Map<Opti
.setFields(FIELDS.getString(options))
.execute();
} catch (IOException ex) {
DnsException serviceException = translate(ex);
DnsException serviceException = translate(ex, true);
if (serviceException.code() == HTTP_NOT_FOUND) {
if ("entity.parameters.changeId".equals(serviceException.location())
|| (serviceException.getMessage() != null
Expand Down Expand Up @@ -190,7 +191,7 @@ public ListResult<Change> listChangeRequests(String zoneName, Map<Option, ?> opt
ChangesListResponse response = request.execute();
return of(response.getNextPageToken(), response.getChanges());
} catch (IOException ex) {
throw translate(ex);
throw translate(ex, true);
}
}
}

0 comments on commit fd740ea

Please sign in to comment.