You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@arrow.apache.org by Siddharth Teotia <si...@dremio.com> on 2019/04/13 18:25:58 UTC
ARROW-3191: Making ArrowBuf work with arbitrary memory
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
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and
setting io.netty.tryReflectionSetAccessible to true for java builds
Posted by Siddharth Teotia <si...@dremio.com>.
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 <cu...@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 <ja...@apache.org> wrote:
>
> > I'm onboard with this change.
> >
> > On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <si...@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 <ja...@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
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and
setting io.netty.tryReflectionSetAccessible to true for java builds
Posted by Bryan Cutler <cu...@gmail.com>.
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 <ja...@apache.org> wrote:
> I'm onboard with this change.
>
> On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <si...@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 <si...@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 <ja...@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 <si...@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 <ja...@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
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and
setting io.netty.tryReflectionSetAccessible to true for java builds
Posted by Jacques Nadeau <ja...@apache.org>.
I'm onboard with this change.
On Fri, Apr 26, 2019 at 2:14 AM Siddharth Teotia <si...@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 <si...@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 <ja...@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 <si...@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 <ja...@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
> >> > > >
> >> > >
> >> >
> >>
> >
>
Re: ARROW-3191: Status update: Making ArrowBuf work with arbitrary memory
Posted by Siddharth Teotia <si...@dremio.com>.
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 <si...@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 <si...@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 <ja...@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 <si...@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 <ja...@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
>>> > > >
>>> > >
>>> >
>>>
>>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory and
setting io.netty.tryReflectionSetAccessible to true for java builds
Posted by Siddharth Teotia <si...@dremio.com>.
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 <si...@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 <ja...@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 <si...@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 <ja...@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
>> > > >
>> > >
>> >
>>
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory
Posted by Siddharth Teotia <si...@dremio.com>.
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 <ja...@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 <si...@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 <ja...@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
> > > >
> > >
> >
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory
Posted by Jacques Nadeau <ja...@apache.org>.
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 <si...@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 <ja...@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 <si...@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
> > >
> >
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory
Posted by Siddharth Teotia <si...@dremio.com>.
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 <ja...@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 <si...@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
> >
>
Re: ARROW-3191: Making ArrowBuf work with arbitrary memory
Posted by Jacques Nadeau <ja...@apache.org>.
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 <si...@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
>