[I] Create a custom thrift protocol to ensure client/server compatibility [accumulo]

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii opened a new issue, #5390:
URL: https://github.com/apache/accumulo/issues/5390

   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](https://en.wikipedia.org/wiki/Magic_number_(programming)) 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:
   
   1. Primarily, create a custom protocol with the versioning information stored in a header and checks on the server side to validate it, then
   2. Secondarily, simplify the thrift APIs by adding tracing info to the protocol header, then lastly
   3. 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)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Create a custom thrift protocol to ensure client/server compatibility [accumulo]

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii closed issue #5390: Create a custom thrift protocol to ensure client/server compatibility
URL: https://github.com/apache/accumulo/issues/5390


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Create a custom thrift protocol to ensure client/server compatibility [accumulo]

["cshannon (via GitHub)" <gi...@apache.org>]

cshannon commented on issue #5390:
URL: https://github.com/apache/accumulo/issues/5390#issuecomment-2714172315

   I think some of these suggestions are a nice improvement. When I was exploring gRPC [prototype](#5000 ) I was starting to look at ways to improve things and make it easier to do tracing and incorporate credentials easier as well. Encapsulating some of this into a custom protocol might make that migration easier if we ever make a switch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Create a custom thrift protocol to ensure client/server compatibility [accumulo]

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on issue #5390:
URL: https://github.com/apache/accumulo/issues/5390#issuecomment-3300236893

   I think item 2 (tracing info) is not really possible or worth pursuing anymore (see https://github.com/apache/accumulo/pull/5398#issuecomment-3300225117), but item 3 (the rpc credentials) are probably still possible to do. We can pass those in the client protocol header the same way we pass the client instanceId.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Create a custom thrift protocol to ensure client/server compatibility [accumulo]

["DomGarguilo (via GitHub)" <gi...@apache.org>]

DomGarguilo commented on issue #5390:
URL: https://github.com/apache/accumulo/issues/5390#issuecomment-2707423097

   I'm interested in working on this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org