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: Status update: Making ArrowBuf work with arbitrary memory
Date Thu, 02 May 2019 04:01:44 GMT
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 <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