arrow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bryan Cutler <cutl...@gmail.com>
Subject Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds
Date Fri, 03 May 2019 17:21:16 GMT
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 <jacques@apache.org> wrote:

> I'm onboard with this change.
>
> On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <siddharth@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 <siddharth@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 <jacques@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 <siddharth@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 <jacques@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 <
> > >> siddharth@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
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message