-
Notifications
You must be signed in to change notification settings - Fork 462
Description
Accumulo currently has a custom Thrift protocol created by a class called TraceProtocolFactory
, which is used by ThriftUtil
to construct transports for our RPCs. This class does nothing more than extend TCompactProtocol
and override two write methods in order to produce a side-effect of capturing trace timing information via opentelemetry on the client side (if tracing is enabled). It doesn't actually modify the RPC message in any way from what TCompactProtocol
does.
This issue proposes that we do modify the message from what TCompactProtocol
does, so that we include a header on every RPC message containing certain information needed on every RPC request to verify it.
At a minimum, the header should include some kind of magic number to uniquely identify the message as one recognizable by servers as the Accumulo protocol, and sufficient version information from the client side of the connection for the server side to verify that the client is a compatible Accumulo version. This header should be validated on the server side part of the protocol when the message is read, and throw an appropriate TException that informs the client that it is not using a recognized/compatible protocol.
In addition, we currently pass a TInfo
object as the first parameter of all of our RPC methods. Then we wrap this with a Proxy
object on the server side with TraceUtil.wrapService()
so that it can create server-side traces. This could be done in the custom protocol that this issue is proposing, instead. If a trace info object was serialized to the header of the custom protocol, then it could be deserialized on the receiving end of the RPC and all of this would be self-contained inside the custom Accumulo protocol, and we could simplify all of our RPCs dramatically.
It may also be possible to serialize the RPC credentials into the header of the protocol as well, to avoid needing to pass that as a separate parameter on every method, too. But I think that bit is low-priority, and could be done as a follow-on task, if at all.
So, the priority is:
- Primarily, create a custom protocol with the versioning information stored in a header and checks on the server side to validate it, then
- Secondarily, simplify the thrift APIs by adding tracing info to the protocol header, then lastly
- Optionally, further simplify the thrift APIs by adding rpc credentials to the protocol header (I'm not sure if this should be done at all, though, so this should definitely be done as a follow-on, if it is done at all)