David Ribeiro Alves has posted comments on this change.
Change subject: Add a RpcContext::RespondFailure() method
Patch Set 6:
bq. wonder whether it would be a clearer API to instead have a flag like
context->DoNotCacheResponse(); which must be called in these cases? It gets a
bit ugly that we have so many different "RespondFailure" code paths now
Yeah, I don't really understand the difference between the names for the
unsuccessful Respond* methods the naming distinction seems forced and
meaningless. IMO these should all have the same name, i.e. be overloads of each
other, explaining in which cases we'd want to use each different overload.
Using context->DoNotCacheResponse() breaks the abstraction in the single-server
rpc case. Even though this might not be ideal I'd still prefer the new:
... set the error
to the alternative:
... set the error.
context->RespondSuccess(); // ?????
Not that the user of this API does not need to know that the response is the
same whether RespondSuccess() and RespondFailure(), it still makes more sense
to call context->RespondFailure() after we've just set an error on the response.
Line 84: // Mark this call as failed but don't actually change the response.
> this isn't clear that it also responds -- "Mark as failed" doesn't have the
Line 87: // to mark the call as failed.
> would be good to explain what actual effect this has
To view, visit http://gerrit.cloudera.org:8080/3191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>