uber / tchannel-java

A Java implementation of the TChannel protocol.
MIT License
134 stars 65 forks source link

Should Request Handler set ResponseCode? #174

Open siddjain opened 7 years ago

siddjain commented 7 years ago

This is more of a question than an issue but I couldn't find another place to ask so asking it as an issue.

My question is that should a request handler be ever setting the ResponseCode? https://github.com/uber/tchannel-java/blob/master/tchannel-core/src/main/java/com/uber/tchannel/api/ResponseCode.java

I looked at following two request handlers: https://github.com/uber/tchannel-java/blob/master/tchannel-example/src/main/java/com/uber/tchannel/thrift/GetValueRequestHandler.java https://github.com/uber/tchannel-java/blob/master/tchannel-example/src/main/java/com/uber/tchannel/thrift/SetValueRequestHandler.java

and notice that they do not set the ResponseCode. It seems to make sense. I suspect the ResponseCode is something that is set by tchannel (in case of an uncaught exception or network error) but just want to confirm.

prashantv commented 7 years ago

If you trace through the builders used to create the Thrift response, you'll see it ends up at Response, which has a default value of OK: https://github.com/uber/tchannel-java/blob/master/tchannel-core/src/main/java/com/uber/tchannel/messages/Response.java#L250

siddjain commented 7 years ago

this does not address the question. please re-read the question to understand what it is asking.

yborovikov commented 6 years ago

@siddjain, yes, the handler must set the response code for non-successful result responses.

result.setSuccess(...);
ThriftReponse response = new ThriftResponse
    .Builder<some_result>(request)
    .setResponseCode(ResponseCode.OK) // this is the default (so can be skipped; i don't)
    .setBody(result)
    .build();
result.setMyDeclaredException(...);
ThriftResponse response = new ThriftResponse
    .Builder<some_result>(request)
    .setResponseCode(ResponseCode.Error)
    .setBody(result)
    .build();