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 >> > > > >> > > >> > >> >
