I'm onboard with this change. On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <siddha...@dremio.com> wrote:
> As part of working on this patch < > https://github.com/apache/arrow/pull/4151>, > I ran into a problem with jdk 9 and 11 builds. Since memory underlying > ArrowBuf may not necessarily be a ByteBuf (or any of its extensions), > methods like nioBuffer() can no longer be delegated as > UnsafeDirectLittleEndian.nioBuffer() to Netty implementation. > > So I used PlatformDependent.directBuffer(memory address, size) to create a > direct Byte Buffer to closely mimic what Netty was originally doing > underneath for nioBuffer(). It turns out that PlatformDependent code in > netty first checks for the existence of constructor DirectByteBuffer(long > address, int size) as seen here > < > https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L223 > >. > The constructor (long address, int size) is very well available in jdk 8, 9 > and 11 but on the next line it tries to set it accessible. The reflection > based access is disabled by default in netty code for jdk >= 9 as seen here > < > https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L829 > >. > Thus calls to PlatformDependent.directBuffer(address, size) were failing in > travis CI builds for JDK 9 and 11 with UnsupportedOperationException as > seen here > < > https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L415 > > > and > this was because of the decision that was taken by netty at startup w.r.t > whether to provide access to constructor or not. > > We should set io.netty.tryReflectionSetAccessible system property to true > in java root pom > > I want to make sure people are aware and agree/disagree with this change. > > The tests now give the following warning: > > WARNING: An illegal reflective access operation has occurred > WARNING: Illegal reflective access by io.netty.util.internal.ReflectionUtil > > (file:/Users/siddharthteotia/.m2/repository/io/netty/netty-common/4.1.22.Final/netty-common-4.1.22.Final.jar) > to constructor java.nio.DirectByteBuffer(long,int) > WARNING: Please consider reporting this to the maintainers of > io.netty.util.internal.ReflectionUtil > WARNING: Use --illegal-access=warn to enable warnings of further illegal > reflective access operations > WARNING: All illegal access operations will be denied in a future release > > Thanks. > On Thu, Apr 18, 2019 at 3:39 PM Siddharth Teotia <siddha...@dremio.com> > wrote: > > > I have made all the necessary changes in java code to work with new > > ArrowBuf, ReferenceManager interfaces. More importantly, there is a > wrapper > > buffer NettyArrowBuf interface to comply with usage in RPC and Netty > > related code. It will be good to get feedback on this one (and of course > > all other changes). As of now, the java modules build fine but I have to > > fix test failures. That is in progress. > > > > On Wed, Apr 17, 2019 at 6:41 AM Jacques Nadeau <jacq...@apache.org> > wrote: > > > >> Are there any other general comments here? If not, let's get this done > and > >> merged. > >> > >> On Mon, Apr 15, 2019, 4:19 PM Siddharth Teotia <siddha...@dremio.com> > >> wrote: > >> > >> > I believe reader/writer indexes are typically used when we send > buffers > >> > over the wire -- so may not be necessary for all users of ArrowBuf. I > >> am > >> > okay with the idea of providing a simple wrapper to ArrowBuf to manage > >> the > >> > reader/writer indexes with a couple of APIs. Note that some APIs like > >> > writeInt, writeLong() bump the writer index unlike setInt/setLong > >> > counterparts. JsonFileReader uses some of these APIs. > >> > > >> > > >> > > >> > On Sat, Apr 13, 2019 at 2:42 PM Jacques Nadeau <jacq...@apache.org> > >> wrote: > >> > > >> > > Hey Sidd, > >> > > > >> > > Thanks for pulling this together. This looks very promising. One > quick > >> > > thought: do we think the concept of the reader and writer index need > >> to > >> > be > >> > > on ArrowBuf? It seems like something that could be added as an > >> additional > >> > > decoration/wrapper when needed instead of being part of the core > >> > structure. > >> > > > >> > > On Sat, Apr 13, 2019 at 11:26 AM Siddharth Teotia < > >> siddha...@dremio.com> > >> > > wrote: > >> > > > >> > > > Hi All, > >> > > > > >> > > > I have put a PR with WIP changes. All the major set of changes > have > >> > been > >> > > > done to decouple the usage of ArrowBuf and reference management. > The > >> > > > ArrowBuf interface is much simpler and clean now. > >> > > > > >> > > > I believe there would be several folks in the community interested > >> in > >> > > these > >> > > > changes so please feel free to take a look at the PR and provide > >> your > >> > > > feedback -- https://github.com/apache/arrow/pull/4151 > >> > > > > >> > > > There is some cleanup needed (code doesn't compile yet) due to > >> moving > >> > the > >> > > > APIs but I have raised the PR to get an early feedback from the > >> > community > >> > > > on the critical changes. > >> > > > > >> > > > Thanks, > >> > > > Siddharth > >> > > > > >> > > > >> > > >> > > >
