Hi Bryan, AFAIK, there is not other impact. So we should be good.
The last few integration issues that I had been chasing are now fixed (got a clean build with my previous commit pushed over the weekend). I just pushed a new commit with some cleanup and the changes are now ready. We should plan to merge this asap this week. Thanks, Siddharth On Fri, May 3, 2019 at 10:21 AM Bryan Cutler <[email protected]> wrote: > 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 <[email protected]> wrote: > > > I'm onboard with this change. > > > > On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <[email protected]> > > 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 <[email protected] > > > > > 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 <[email protected]> > > > 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 < > [email protected]> > > > >> 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 < > [email protected]> > > > >> 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 < > > > >> [email protected]> > > > >> > > 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 > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > >
