READY: Perf/query ttb encoding

[erikleitch]

---

Context

This is part of a set of related PRs to rework handling of PB vs TTB encoding for client/server messages:

• https://github.com/basho/riak-erlang-client/pull/267
• https://github.com/basho/riak_pb/pull/182
• https://github.com/basho/riak_api/pull/109
• https://github.com/basho/riak_kv/pull/1378

The original addition of alternate encodings was a hybrid implementation which 1) required the client and server to agree via a handshaking protocol about what encoding would be used, 2) used messages for both encodings that were generated from the .proto files, but some were only used for TTB encoding, and 3) allowed the server to send PB-encoded responses to TTB-encoded requests.

This work is an attempt to rationalize this situation, and to simplify how differently-encoded messages are handled within riak. The guiding principles are:

1. Even as we optimize encodings for RiakTS performance, the server should continue to support PB-encoding as before. We do not, for example, want to break custom PB connections made outside our supported client codebase
2. We should be able to optimize messages for whatever encoding is imposed on them.
3. The server should handle interleaved messages with different encodings seamlessly, without any handshaking about encoding type
4. We should guarantee that responses are encoded in the same way as the requests that generated them

---

Notes

• Regarding point 2. above, although we currently pack data into the same containers differently for PB vs TTB encoding, we do not currently have a use-case that requires separate message structures, so the decision was made to keep the existing .proto-generated messages until we have such a use-case.
• Because TTB encoding is self-describing, we do not need to switch on per-message 8-bit MsgCode, as with PB-encoded messages, to know how to decode them. This work therefor adds a single new MsgCode (104) to indicate that a message is TTB-encoded. This allows the server to determine what encoding is being used per-message and without handshaking, addressing point 3. above. It also preserves the remaining 8-bit space for future messages if needed.
• The server now implements separate services for PB and TTB-encoded messages, selected on MsgCode. Because the processing is done per-service, and not per-message, this also guarantees that responses are encoded in the same way as the requests, addressing point 4 above. This is important, for example, for tsquery requests, where it is the encoding of the responses, not the requests that matters for performance.

---

Changes in this repo:

This change removes the need to synchronize encoding schemes between client and server that was accomplished by sending the PB_TOGGLE_ENCODING message. Changes to riak_api_pb_server.erl simply remove all code that deals with the now-obsolete handshaking.

[lukebakken]

Take all of this with a grain of salt, it's just me being pedantic:

• The following is no longer a PB specific server, and should be renamed to reflect that it's really a TCP one: https://github.com/basho/riak_api/blob/perf/query_ttb_encoding/src/riak_api_pb_server.erl
• MsgCode 104 would then be used here to select the TTB codec service, not the PB one: https://github.com/basho/riak_api/blob/perf/query_ttb_encoding/src/riak_api_pb_server.erl#L212-L219
• Other files / supervisors would be renamed to tcp instead of pb.

[hazen]

@lukebakken We'd still want to make the changes Erik is prosing here. You'd just take it one step further by renaming to a TCP server, right?

[lukebakken]

@javajolt yeah these changes are minor. I started work on basho/riak_kv#1379 today and will get that done tomorrow.

[erikleitch]

I think the renaming proposed here is ok, however I think we should table any further pedantry for a post-TS1.3-release timeframe. (Technically the whole riak_pb repo is now a misnomer, and should be riak_tcp, or a separate repo should be created for other encodings, etc., but these are changes that would break lots of rebar.config dependencies, builds, etc)

Actually, I don't think even this renaming is advisable at this point. riak_api_pb_XXX naming convention is used all over riak_kv and yokozuna. I would suggest a separate effort to renormalize our naming scheme when we are not a few days out from a release.

[alexmoore]

+1 03471df

[alexmoore]

@borshop merge