Thanks for the comments Li. For your concerns about memory ownership, I don't think anything is really changed here, but we can discuss further in the PR. I'm not sure I quite understand your concern when you say "complexity of maintaining both style APIs"? The proposed changes are for 1 coherent API, not 2, and I think it simplifies things for the user and our codebase. The holder object is just a simple data struct to allow returning all of the message info, including the message, to the user. I would really like to get this API right this time to avoid any more changes, so suggestions for improvements would be much appreciated.
Thanks, Bryan On Sun, Jul 15, 2018 at 12:56 PM, Li Jin <ice.xell...@gmail.com> wrote: > Bryan, > > Sorry for the delay. I did a round of review of the API change (high > level). > > I understand the proposed API changes allows users of the Arrow Java > library to implement Arrow reader that reads Arrow data to on-heap memory > directly. However, my main feedback is > that the proposed API changes introduced a new style of read API - instead > of return the Arrow message and data directly from the read method, there > are new APIs introduced to read the message into a "holder" object. I am a > little concerned about the complexity of maintaining both style APIs. There > is another concern about memory ownership, which I have replied in more > details in the PR - the buffer allocator is moved into the message reader > class itself, instead of being provided by the caller - this could cause > some confusion about memory ownership. > > That being said, I feel I am not 100% confident to say +1, +0 or -1 to the > proposed change alone and I request some other Java committer review this > change with me together. > > Thank you, > Li > > On Mon, Jul 9, 2018 at 11:50 PM, Li Jin <ice.xell...@gmail.com> wrote: > > > Bryan, > > > > Sorry I am traveling now but I will try to take to look in the next few > > days. > > > > Li > > > > On Mon, Jul 9, 2018 at 11:16 PM, Bryan Cutler <cutl...@gmail.com> wrote: > > > >> Hi All, > >> > >> I'm proposing some Java API changes to MessageReader, with minor changes > >> to > >> ArrowStreamReader and MessageSerializer, as part of ARROW-2704 [1] and > can > >> be seen in the PR [2]. These changes are to improve processing an Arrow > >> stream on a per Message basis. > >> > >> A while ago I introduced the MessageReader interface in anticipation of > >> some future work, but it fell a little short of what I needed. So after > >> tweaking the APIs a bit, it can now allow the user to implement message > >> stream processing without knowing specifics about the stream format and > in > >> an optimal way that avoids unnecessary buffer copying. If you have some > >> concerns about these APIs, please discuss in the PR at [2]. If no one > >> objects, it would be great to get these changes in version 0.10.0. > >> Thanks! > >> > >> Bryan > >> > >> > >> [1]: https://issues.apache.org/jira/browse/ARROW-2704 > >> [2]: https://github.com/apache/arrow/pull/2139 > >> > > > > >
