RB is not working for me at mo so reviews here:
I asked about comment in ClientCache in previous reviews but it went unanswered. I'm talking about the old Doug comment on a. vs. b. choice. I didn't understand it then and less so now. Is it a copy/paste error?
The writing to a buffer before we write on the line goes away if we move to a pure pb engine?
Is the size used here right for case where we have an exception? (Or its just a hint?)
ByteBufferOutputStream buf = new ByteBufferOutputStream(size);
And RPCRequestWritable and RPCResponseWritable will go away too when we do pb? (You've answered yes to this multiple times I believe).
Why the need for a setRPC w/ no params?
This stuff passed to Server, whats it about? Is this some Hadoop thing about being able to do multiple serializations?
Class<? extends Writable> paramClass,
When we do logResponse, we no longer take a Call but we pass in its name and methodnames and params.... is that a regression? Would it be better to take the Invocation?
The below doesn't look too bad:
In header can we say if its pbs that follow? Or will we have to always calc message size before sending?
Why we need this: clientProtocolVersion in request? Is this Writables thing? It goes away when we go pb?
My general thought on this is we commit. This patch has same shape as our current rpc'ing; same classes, etc. just moved over some to support pb. We need to go pure pb and get rid of Writables in this rpc call path. I see this as a stepping stone so we should get it in so we can undo it later w/ pb engine.