Quick status update: I have 2 outstanding integration test failures <https://travis-ci.org/apache/arrow/builds/524723636?utm_source=github_status&utm_medium=notification>that need to be addressed -- was out for a couple of days and then got dragged into another issue. Looking into the failures now. I hope people have looked at my previous email for the change I had made to get the jdk >= 9 builds passing.
On Thu, Apr 25, 2019 at 3:13 PM 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 >>> > > > >>> > > >>> > >>> >>
