Conversation
| return grpcCode.toStatus(); | ||
| } catch (IllegalArgumentException e) { | ||
| return Status.UNKNOWN.withDescription("Unrecognized StatusCode: " + code); | ||
| } |
There was a problem hiding this comment.
Should I use explicit 1:1 mapping instead:
switch (code) {
case OK:
return Status.OK;
case CANCELLED:
return Status.CANCELLED;
case UNKNOWN:
return Status.UNKNOWN;
case INVALID_ARGUMENT:
return Status.INVALID_ARGUMENT;
case DEADLINE_EXCEEDED:
return Status.DEADLINE_EXCEEDED;
case NOT_FOUND:
return Status.NOT_FOUND;
case ALREADY_EXISTS:
return Status.ALREADY_EXISTS;
case PERMISSION_DENIED:
return Status.PERMISSION_DENIED;
case RESOURCE_EXHAUSTED:
return Status.RESOURCE_EXHAUSTED;
case FAILED_PRECONDITION:
return Status.FAILED_PRECONDITION;
case ABORTED:
return Status.ABORTED;
case OUT_OF_RANGE:
return Status.OUT_OF_RANGE;
case UNIMPLEMENTED:
return Status.UNIMPLEMENTED;
case INTERNAL:
return Status.INTERNAL;
case UNAVAILABLE:
return Status.UNAVAILABLE;
case DATA_LOSS:
return Status.DATA_LOSS;
case UNAUTHENTICATED:
return Status.UNAUTHENTICATED;
default:
return Status.UNKNOWN.withDescription("Unknown StatusCode: " + code);
}
There was a problem hiding this comment.
StatusCode comes from the com.google.api.gax.rpc package, and Status comes from the io.grpc package. It's unlikely, but possible, that enums change in one release of one package, and not get updated at the same time in the other package, or the names don't match, or that we don't upgrade our dependencies at the same time for both packages, etc. The downside of adding the switch statement is having to upgrade the switch statement every time there's a change in the enums.
I'm leaning towards keeping it the way you have done it, especially because we're not going to keep an eye on the status code changes (for example if a new code is added, we'll forget about adding it here). Without the switch statement things should settle down and continue to work together after both packages are upgraded.
There was a problem hiding this comment.
make sense. thank you Ehsan
ehsannas
left a comment
There was a problem hiding this comment.
LGTM. One nit pointed out below.
| return grpcCode.toStatus(); | ||
| } catch (IllegalArgumentException e) { | ||
| return Status.UNKNOWN.withDescription("Unrecognized StatusCode: " + code); | ||
| } |
There was a problem hiding this comment.
StatusCode comes from the com.google.api.gax.rpc package, and Status comes from the io.grpc package. It's unlikely, but possible, that enums change in one release of one package, and not get updated at the same time in the other package, or the names don't match, or that we don't upgrade our dependencies at the same time for both packages, etc. The downside of adding the switch statement is having to upgrade the switch statement every time there's a change in the enums.
I'm leaning towards keeping it the way you have done it, especially because we're not going to keep an eye on the status code changes (for example if a new code is added, we'll forget about adding it here). Without the switch statement things should settle down and continue to work together after both packages are upgraded.
|
|
||
| package com.google.cloud.firestore; | ||
|
|
||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
style guide disallows using .* imports.
While creating a FirestoeException from an ApiException, it does not set
statusproperty, which is required by client side metrics feature.To be able to correctly collect an operation status(OK, DEADLINE_EXCEEDED,ABORTED...) instead of defaulting it to UNKNOWN, this PR maps StatusCode to Status, based on the 1:1 mapping relationship between enum
StatusCode.CodeandStatus.Code, while creating FirestoreException from an ApiException.