Hi Sidd, Does setting the system property io.netty.tryReflectionSetAccessible to true have any other adverse effect other than those warnings during build?
Bryan On Thu, May 2, 2019 at 8:43 PM Jacques Nadeau <jacq...@apache.org> wrote: > 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 > > >> > > > > > >> > > > > >> > > > >> > > > > > >
