Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Marked exceptions for create and deletes in DNS as non-idempotent. #883

Merged
merged 5 commits into from
Apr 14, 2016
Merged

Marked exceptions for create and deletes in DNS as non-idempotent. #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a20c01
Marked exceptions for create and deletes as non-idempotent.
mderka Apr 8, 2016
81362af
Added error flag for indicating if the request was rejected.
mderka Apr 9, 2016
3b8631f
Minor cleanup refactoring.
mderka Apr 11, 2016
c3647d0
Minor code cleanup.
mderka Apr 14, 2016
2a11dbf
Switched create back to idempotent, comment adjustments
mderka Apr 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 32 additions & 28 deletions gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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);
}
}
}