Skip to content

Commit fd740ea

Browse files
mderkaaozarov
mderka
authored and
aozarov
committed
Marked exceptions for create and deletes in DNS as non-idempotent. (#883)
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.
1 parent 13ab36f commit fd740ea

File tree

3 files changed

+52
-47
lines changed

3 files changed

+52
-47
lines changed

‎gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java‎

Lines changed: 32 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,16 @@ protected static final class Error implements Serializable {
4848

4949
private final Integer code;
5050
private final String reason;
51+
private final boolean rejected;
5152

5253
public Error(Integer code, String reason) {
54+
this(code, reason, false);
55+
}
56+
57+
public Error(Integer code, String reason, boolean rejected) {
5358
this.code = code;
5459
this.reason = reason;
60+
this.rejected = rejected;
5561
}
5662

5763
/**
@@ -61,18 +67,27 @@ public Integer code() {
6167
return code;
6268
}
6369

70+
/**
71+
* Returns true if the error indicates that the API call was certainly not accepted by the
72+
* server. For instance, if the server returns a rate limit exceeded error, it certainly did not
73+
* process the request and this method will return {@code true}.
74+
*/
75+
public boolean rejected() {
76+
return rejected;
77+
}
78+
6479
/**
6580
* Returns the reason that caused the exception.
6681
*/
6782
public String reason() {
6883
return reason;
6984
}
7085

71-
boolean isRetryable(Set<Error> retryableErrors) {
86+
boolean isRetryable(boolean idempotent, Set<Error> retryableErrors) {
7287
for (Error retryableError : retryableErrors) {
7388
if ((retryableError.code() == null || retryableError.code().equals(this.code()))
7489
&& (retryableError.reason() == null || retryableError.reason().equals(this.reason()))) {
75-
return true;
90+
return idempotent || retryableError.rejected();
7691
}
7792
}
7893
return false;
@@ -95,12 +110,14 @@ public BaseServiceException(IOException exception, boolean idempotent) {
95110
String reason = null;
96111
String location = null;
97112
String debugInfo = null;
113+
Boolean retryable = null;
98114
if (exception instanceof GoogleJsonResponseException) {
99115
GoogleJsonError jsonError = ((GoogleJsonResponseException) exception).getDetails();
100116
if (jsonError != null) {
101-
Error error = error(jsonError);
117+
Error error = new Error(jsonError.getCode(), reason(jsonError));
102118
code = error.code;
103119
reason = error.reason;
120+
retryable = isRetryable(idempotent, error);
104121
if (reason != null) {
105122
GoogleJsonError.ErrorInfo errorInfo = jsonError.getErrors().get(0);
106123
location = errorInfo.getLocation();
@@ -110,22 +127,16 @@ public BaseServiceException(IOException exception, boolean idempotent) {
110127
code = ((GoogleJsonResponseException) exception).getStatusCode();
111128
}
112129
}
130+
this.retryable = MoreObjects.firstNonNull(retryable, isRetryable(idempotent, exception));
113131
this.code = code;
114-
this.retryable = idempotent && isRetryable(exception);
115132
this.reason = reason;
116133
this.idempotent = idempotent;
117134
this.location = location;
118135
this.debugInfo = debugInfo;
119136
}
120137

121138
public BaseServiceException(GoogleJsonError error, boolean idempotent) {
122-
super(error.getMessage());
123-
this.code = error.getCode();
124-
this.reason = reason(error);
125-
this.idempotent = idempotent;
126-
this.retryable = idempotent && isRetryable(error);
127-
this.location = null;
128-
this.debugInfo = null;
139+
this(error.getCode(), error.getMessage(), reason(error), idempotent);
129140
}
130141

131142
public BaseServiceException(int code, String message, String reason, boolean idempotent) {
@@ -138,7 +149,7 @@ public BaseServiceException(int code, String message, String reason, boolean ide
138149
this.code = code;
139150
this.reason = reason;
140151
this.idempotent = idempotent;
141-
this.retryable = idempotent && new Error(code, reason).isRetryable(retryableErrors());
152+
this.retryable = isRetryable(idempotent, new Error(code, reason));
142153
this.location = null;
143154
this.debugInfo = null;
144155
}
@@ -147,15 +158,12 @@ protected Set<Error> retryableErrors() {
147158
return Collections.emptySet();
148159
}
149160

150-
protected boolean isRetryable(GoogleJsonError error) {
151-
return error != null && error(error).isRetryable(retryableErrors());
161+
protected boolean isRetryable(boolean idempotent, Error error) {
162+
return error.isRetryable(idempotent, retryableErrors());
152163
}
153164

154-
protected boolean isRetryable(IOException exception) {
155-
if (exception instanceof GoogleJsonResponseException) {
156-
return isRetryable(((GoogleJsonResponseException) exception).getDetails());
157-
}
158-
return exception instanceof SocketTimeoutException;
165+
protected boolean isRetryable(boolean idempotent, IOException exception) {
166+
return idempotent && exception instanceof SocketTimeoutException;
159167
}
160168

161169
/**
@@ -187,8 +195,8 @@ public boolean idempotent() {
187195
}
188196

189197
/**
190-
* Returns the service location where the error causing the exception occurred. Returns
191-
* {@code null} if not set.
198+
* Returns the service location where the error causing the exception occurred. Returns {@code
199+
* null} if not available.
192200
*/
193201
public String location() {
194202
return location;
@@ -223,18 +231,14 @@ public int hashCode() {
223231
debugInfo);
224232
}
225233

226-
protected static String reason(GoogleJsonError error) {
227-
if (error.getErrors() != null && !error.getErrors().isEmpty()) {
234+
private static String reason(GoogleJsonError error) {
235+
if (error.getErrors() != null && !error.getErrors().isEmpty()) {
228236
return error.getErrors().get(0).getReason();
229237
}
230238
return null;
231239
}
232240

233-
protected static Error error(GoogleJsonError error) {
234-
return new Error(error.getCode(), reason(error));
235-
}
236-
237-
protected static String message(IOException exception) {
241+
private static String message(IOException exception) {
238242
if (exception instanceof GoogleJsonResponseException) {
239243
GoogleJsonError details = ((GoogleJsonResponseException) exception).getDetails();
240244
if (details != null) {

‎gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsException.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,16 @@ public class DnsException extends BaseServiceException {
3131

3232
// see: https://cloud.google.com/dns/troubleshooting
3333
private static final Set<Error> RETRYABLE_ERRORS = ImmutableSet.of(
34-
new Error(429, null),
35-
new Error(500, null),
36-
new Error(502, null),
37-
new Error(503, null),
38-
new Error(null, "userRateLimitExceeded"),
39-
new Error(null, "rateLimitExceeded"));
34+
new Error(429, null, true),
35+
new Error(500, null, false),
36+
new Error(502, null, false),
37+
new Error(503, null, false),
38+
new Error(null, "userRateLimitExceeded", true),
39+
new Error(null, "rateLimitExceeded", true));
4040
private static final long serialVersionUID = 490302380416260252L;
4141

42-
public DnsException(IOException exception) {
43-
super(exception, true);
42+
public DnsException(IOException exception, boolean idempotent) {
43+
super(exception, idempotent);
4444
}
4545

4646
private DnsException(int code, String message) {

‎gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/DefaultDnsRpc.java‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ public class DefaultDnsRpc implements DnsRpc {
3636
private final Dns dns;
3737
private final DnsOptions options;
3838

39-
private static DnsException translate(IOException exception) {
40-
return new DnsException(exception);
39+
private static DnsException translate(IOException exception, boolean idempotent) {
40+
return new DnsException(exception, idempotent);
4141
}
4242

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

@@ -73,7 +74,7 @@ public ManagedZone getZone(String zoneName, Map<Option, ?> options) throws DnsEx
7374
.setFields(FIELDS.getString(options))
7475
.execute();
7576
} catch (IOException ex) {
76-
DnsException serviceException = translate(ex);
77+
DnsException serviceException = translate(ex, true);
7778
if (serviceException.code() == HTTP_NOT_FOUND) {
7879
return null;
7980
}
@@ -93,7 +94,7 @@ public ListResult<ManagedZone> listZones(Map<Option, ?> options) throws DnsExcep
9394
.execute();
9495
return of(zoneList.getNextPageToken(), zoneList.getManagedZones());
9596
} catch (IOException ex) {
96-
throw translate(ex);
97+
throw translate(ex, true);
9798
}
9899
}
99100

@@ -103,7 +104,7 @@ public boolean deleteZone(String zoneName) throws DnsException {
103104
dns.managedZones().delete(this.options.projectId(), zoneName).execute();
104105
return true;
105106
} catch (IOException ex) {
106-
DnsException serviceException = translate(ex);
107+
DnsException serviceException = translate(ex, false);
107108
if (serviceException.code() == HTTP_NOT_FOUND) {
108109
return false;
109110
}
@@ -126,7 +127,7 @@ public ListResult<ResourceRecordSet> listRecordSets(String zoneName, Map<Option,
126127
.execute();
127128
return of(response.getNextPageToken(), response.getRrsets());
128129
} catch (IOException ex) {
129-
throw translate(ex);
130+
throw translate(ex, true);
130131
}
131132
}
132133

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

@@ -148,7 +149,7 @@ public Change applyChangeRequest(String zoneName, Change changeRequest, Map<Opti
148149
.setFields(FIELDS.getString(options))
149150
.execute();
150151
} catch (IOException ex) {
151-
throw translate(ex);
152+
throw translate(ex, false);
152153
}
153154
}
154155

@@ -160,7 +161,7 @@ public Change getChangeRequest(String zoneName, String changeRequestId, Map<Opti
160161
.setFields(FIELDS.getString(options))
161162
.execute();
162163
} catch (IOException ex) {
163-
DnsException serviceException = translate(ex);
164+
DnsException serviceException = translate(ex, true);
164165
if (serviceException.code() == HTTP_NOT_FOUND) {
165166
if ("entity.parameters.changeId".equals(serviceException.location())
166167
|| (serviceException.getMessage() != null
@@ -190,7 +191,7 @@ public ListResult<Change> listChangeRequests(String zoneName, Map<Option, ?> opt
190191
ChangesListResponse response = request.execute();
191192
return of(response.getNextPageToken(), response.getChanges());
192193
} catch (IOException ex) {
193-
throw translate(ex);
194+
throw translate(ex, true);
194195
}
195196
}
196197
}

0 commit comments

Comments
 (0)