Hi guys, Thanks for sharing your insights/concerns! I also left some comments based on the discussion we had. Briefly:
1. It seems like renaming stream_id to schema_id and delegating "logical stream" distinction to app_metadata mitigates the "multiplexing" point while at the same time it gives enough flexibility to address both Nate's and our use cases. 2. To David's point about other transports: in fact currently we are using other transports(aside from gRPC) so we don't wanna depend on only gRPC features. Cheers, Gosh On Wed, Jun 23, 2021 at 10:40 PM David Li <lidav...@apache.org> wrote: > Thanks for chiming in - I've replied in the doc. Scoping it to just schema > evolution would be preferable, but I'm not sure if Gosh's usecase requires > more flexibility than that or not. > > Again, though, given that 1) gRPC recycles a connection, so repeated calls > aren't necessarily expensive and 2) encoding tricks like union-of-structs, > any solution needs to be weighed against those/we should make sure to > document why they aren't sufficient. (For instance, 1) is hampered by the > use of L7 load balancers and/or client-side load balancing policies in gRPC > and assumes statefulness which is undesirable in general. There's also the > eventual desire to have a transport besides gRPC someday.) > > -David > > On Wed, Jun 23, 2021, at 16:24, Nate Bauernfeind wrote: > > Thanks for writing this up! I added a few general comments, but have a > question on the approach because it's not quite what I was expecting. > > I am slightly concerned that the proposal looks more like support for > "multiplexing" IPC streams into a single RPC stream rather than support for > a changing Schema of an otherwise consistently logical stream. gRPC already > does a good job decoupling RPC streams from one another. I feel that > throwing that idea into the IPC stream increases client-library > implementation cost by quite a lot. > > Why is it not good enough to replace the Schema when we see a duplicate? > This is undoubtedly less work across all client implementations. > > The benefit I see is that you might have two schemas that you swap between > frequently then you can indicate with a single integer. If that's what you > want to support I would rather think of them as `schema_id` instead of > `stream_id` and not give this impression that multiplexing is a goal. As > you have proposed, it seems that the "done writing for a stream" needs a > callback notifying the user receiving the stream that a logical subset of > the flight is complete. Alternatively, if they aren't independent streams > (to the end-user), we could tell the Arrow layer that a particular schema > is no longer needed without also needing to communicate further downstream. > > On Wed, Jun 23, 2021 at 1:39 PM David Li <lidav...@apache.org> wrote: > > > Ah to be clear, the API is indeed inconsistent - DoExchange was added > some > > time later (and by its nature returning a FlightDataStream would not have > > been possible, since it's meant to be able to interleave > reading/writing). > > But really, DoGet is indeed the odd one out in the C++ API and it may be > > worth correcting. You could also perhaps imagine making a > FlightDataStream > > implementation that accepts a closure and provides it a fake writer, if > the > > API mismatch is hard to work with... > > > > That said: this has some benefits, e.g. for a Python service that returns > > a Table, that means data can be fed into gRPC entirely in C++ without > > having to bounce into Python for each chunk. > > > > Best, > > David > > > > On Wed, Jun 23, 2021, at 15:33, Gosh Arzumanyan wrote: > > > Hi David, > > > > > > Got you. In fact I was looking at this more from the point of view of > > consistency of the API in terms of "inputs" and thought DoExchange is > kind > > of a DoGet+ so might make sense to have the same classes being utilized > in > > both places. But again, I might be missing something and I get the point > > about breaking change. > > > > > > Cheers, > > > Gosh > > > > > > On Wed, Jun 23, 2021 at 2:58 PM David Li <lidav...@apache.org> wrote: > > >> __ > > >> It's mostly a quirk of implementation (and just for clarification, > > they're all nearly identical on the format/protocol level). > > >> > > >> DoGet is conceptualized as your application returning a readable > stream > > of batches, instead of your application imperatively writing batches to > the > > client. (This is different than how Flight is implemented in Java.) You > > would normally not implement FlightDataStream - you would return a > > RecordBatchStream. > > >> > > >> DoGet could not have FlightMessageWriter as a return type as that > > wouldn't make sense, but it could accept an instance of that as a > parameter > > instead, much like DoExchange. That would be a breaking change. > > >> > > >> Best, > > >> David > > >> > > >> On Wed, Jun 23, 2021, at 08:47, Gosh Arzumanyan wrote: > > >>> Hi David, > > >>> > > >>> Going through the ArrowFlight API: got confused a bit on DoGet and > > >>> DoPut/DoExachange apis: the former one expects FlightDataStream which > > talks > > >>> in already serialized message terms while the latter to > > >>> accept FlightMessageReader/Writer which expect the user to pass in > > >>> RecordBatches etc. Is there any reason why the DoGet can't have > > >>> FlightMessageWriter as a return type? > > >>> > > >>> Cheers, > > >>> Gosh > > >>> > > >>> On Mon, Jun 21, 2021 at 9:47 PM Gosh Arzumanyan <gosh...@gmail.com> > > wrote: > > >>> > > >>> > Thanks David! > > >>> > > > >>> > I also responded/added more suggestions/questions to the doc. I > > think it > > >>> > makes sense to have two sections: one purely protocol oriented and > > second > > >>> > API oriented(examples in c++ or in any other language should make > > the idea > > >>> > easier to digest). > > >>> > > > >>> > Thanks for the reference too! > > >>> > > > >>> > Cheers, > > >>> > Gosh > > >>> > > > >>> > On Mon, Jun 21, 2021 at 4:41 PM David Li <lidav...@apache.org> > > wrote: > > >>> > > > >>> >> Thanks! I've left some initial comments/suggestions to expand it > in > > terms > > >>> >> of the format definitions and not the C++ APIs. > > >>> >> > > >>> >> I'll also note something like this was proposed a long time ago - > > there's > > >>> >> not very much discussion about it there but for reference: > > >>> >> > > > https://lists.apache.org/thread.html/0e5ba78c48cdd0e357f3a4a6d8affd31767c34376b62c001910823af%40%3Cdev.arrow.apache.org%3E > > >>> >> (or see the thread '[Discuss][FlightRPC] Extensions to Flight: > > >>> >> "DoBidirectional"' from 2019-2020). It might be good to address > why > > the > > >>> >> proposed workaround there (union-of-structs) is insufficient for > > the use > > >>> >> cases here (and in FlightSQL). > > >>> >> > > >>> >> -David > > >>> >> > > >>> >> On Mon, Jun 21, 2021, at 08:22, Gosh Arzumanyan wrote: > > >>> >> > Ah sorry, comments should work now. > > >>> >> > > > >>> >> > Cheers, > > >>> >> > Gosh > > >>> >> > > > >>> >> > On Mon., 21 Jun. 2021, 14:18 David Li, <lidav...@apache.org > > <mailto: > > >>> >> lidavidm%40apache.org>> wrote: > > >>> >> > > > >>> >> > > Thanks! Will give it a look. > > >>> >> > > > > >>> >> > > Would you mind opening it up for comments? > > >>> >> > > > > >>> >> > > -David > > >>> >> > > > > >>> >> > > On 2021/06/21 11:56:24, Gosh Arzumanyan <gosh...@gmail.com > > <mailto: > > >>> >> gosharz%40gmail.com>> wrote: > > >>> >> > > > Hi folks, > > >>> >> > > > > > >>> >> > > > Started putting some thoughts together here: > > >>> >> > > > > > >>> >> > > > > >>> >> > > > https://docs.google.com/document/d/1dIOpKNYwsd9sdChsRBAx37BiJXl_7enpwWkH76n1tOI/edit?usp=sharing > > >>> >> > > > Any feedback is welcome! > > >>> >> > > > > > >>> >> > > > Cheers, > > >>> >> > > > Gosh > > >>> >> > > > > > >>> >> > > > > >>> >> > > > >>> >> > > >>> > > > >>> > > >> > > > > > -- > > >
