GH-37937: [C++][FlightRPC] Investigate using gRPC's generic API using gRPC's BidiReactor#49339
GH-37937: [C++][FlightRPC] Investigate using gRPC's generic API using gRPC's BidiReactor#49339raulcd wants to merge 7 commits intoapache:mainfrom
Conversation
|
|
|
Some initial discussion happened on this same branch on a PR my fork, see more details here: I am moving the PR to here for visibility. |
raulcd
left a comment
There was a problem hiding this comment.
@lidavidm @pitrou I've spent some time today learning more about our FlightDataSerialize method and how we could expose an API that leaves gRPC out of the picture using a BufferVector instead. This is only for the write path so far but I wanted to share early to validate this is in-line with what we had been discussing.
…ges. Build new PayloadData::SerializeToBuffers method to retrieve list of arrow::buffers from a PayloadData. This function internally is a copy on what we had on FlightDataSerialize but using arrow buffers instead of grpc::ByteBuffers. Move the logic to a single place and reuse on FlightDataSerialize
…tly with arrow::Buffer and FlightData
…entation that consumes arrow::buffers and triggers calls to a user built listener once RecordBatch has been read
2deb7c8 to
f88832c
Compare
|
@lidavidm @pitrou I've moved the serialization / Deserialization logic from /// \brief Serialize a FlightPayload to a vector of buffers.
///
/// The first buffer contains the protobuf wire format header. Subsequent
/// buffers are zero-copy references to the IPC body buffers, with padding
/// buffers inserted as needed for 8-byte alignment.
arrow::Result<arrow::BufferVector> SerializePayloadToBuffers(
const FlightPayload& payload);
/// \brief Deserialize FlightData from a contiguous buffer.
arrow::Result<FlightData> DeserializeFlightData(
const std::shared_ptr<arrow::Buffer>& buffer);The external API to be used is either I've adapted the gRPC Async PoC to show how this would work with the bidi reactor. The only point where I am not entirely clear is whether utilities specific to gRPC should be exposed. This matters for users building their own bidi reactors (see the async grpc example on the PR). If our stand-point is that users /// Convert an Arrow Buffer to a gRPC Slice.
arrow::Result<::grpc::Slice> SliceFromBuffer(const std::shared_ptr<arrow::Buffer>& buf);
/// Wrap a gRPC ByteBuffer as a zero-copy Arrow Buffer (and clear the ByteBuffer).
arrow::Status WrapGrpcBuffer(::grpc::ByteBuffer* grpc_buf,
std::shared_ptr<arrow::Buffer>* out);I'd like to move this forward, maybe an initial PR only moving the serialization / deserialization logic to handle |
Warning
Do not merge, this is a PoC being discussed at the moment
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
This PR includes breaking changes to public APIs. (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)