Move some exception translation logic to BaseServiceException

[mziccard]

In DefaultStorageRpc (and also other services as DefaultBigQueryRpc) we have code handling the translation of IOException to service specific exceptions:

private static BigQueryException translate(IOException exception) {
  BigQueryException translated;
  if (exception instanceof GoogleJsonResponseException
      && ((GoogleJsonResponseException) exception).getDetails() != null) {
    translated = translate(((GoogleJsonResponseException) exception).getDetails());
  } else {
    translated =
        new BigQueryException(BigQueryException.UNKNOWN_CODE, exception.getMessage(), false);
  }
  translated.initCause(exception);
  return translated;
}

private static BigQueryException translate(GoogleJsonError exception) {
  boolean retryable = RETRYABLE_CODES.contains(exception.getCode());
  return new BigQueryException(exception.getCode(), exception.getMessage(), retryable);
}

Part of this logic should be moved to BaseServiceException

[aozarov]

When we do so, we should probably also add a boolean parameter to indicate if the failed operation is idempotent. The retryable property should be true only if operation is idempotent (passed by the caller to translate) and error code (could be derived from a set passed in the constructor or subclasses) is retryable.

An example of a non-idempotent operation could be pubsub publish as messages do not include id and a retry can cause the same message to be published more than once (though one can argue that because
pub-sub is a "at-least-once message delivery" system that should not matter, so maybe this is not a great example).

[mziccard]

In DefaultStorageRpc to say if an exception is retryable we check whether exception's error code is in RETRYABLE_CODES, but we also check whether the error message is "InternalError":

private static StorageException translate(GoogleJsonError exception) {
  boolean retryable = RETRYABLE_CODES.contains(exception.getCode())
      || "InternalError".equals(exception.getMessage());
  return new StorageException(exception.getCode(), exception.getMessage(), retryable);
}

Looking here that error message should be only associated to 500 errors. Is there a reason I am missing for checking it?

[aozarov]

see "InternalError" associated with a 403.

[mziccard]

Oops, sorry I missed that. This 'peculiar' behavior of each service makes hard to move all translation logic to ServiceException (e.g. testing if error code is in a set of retryable codes is not enough for storage as we also need to check the message). Checking for "InternalError" message for services as BigQuery would do no harm but I'm not sure moving such code to ServiceException is the best choice.

[aozarov]

There are various ways this could be handled. One option is to make an abstract isRetryable method that accepts a GoogleJsonError. most base service specific implementation would just have

@Override
protected boolean isRetryable(GoogleJsonError exception) {
  return RETRYABLE_CODES.contains(exception.getCode());
}

where as StorageException would override it as

@Override
protected boolean isRetryable(GoogleJsonError exception) {
  return RETRYABLE_CODES.contains(exception.getCode()) || "InternalError".equals(exception.getMessage());
}

If you were thinking to somehow provide the set of error codes (e.g. by overriding a method retryableErrorCodes) then you can also make the common implementation for isRetryable
overridable and StorageExcetpion will override it and inside return

return super.isRetryable(exception) || "InternalError".equals(exception.getMessage());

[aozarov]

As part of this change we could simply (and maybe use a common) the service's EXCEPTION_HANDLER_INTERCEPTOR Interceptor.

[mziccard]

This was fixed in #554