arrow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Siddharth Teotia <siddha...@dremio.com>
Subject Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and setting io.netty.tryReflectionSetAccessible to true for java builds
Date Mon, 06 May 2019 18:17:48 GMT
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 <cutlerb@gmail.com> 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 <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