You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jduo (via GitHub)" <gi...@apache.org> on 2023/11/24 14:40:58 UTC
[PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
jduo opened a new pull request, #38876:
URL: https://github.com/apache/arrow/pull/38876
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo closed pull request #38876: GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System
URL: https://github.com/apache/arrow/pull/38876
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829853654
I'm not going to be able to review this for a while.
FWIW, I would be fine deferring other modules for another PR, and I would also be fine saying that arrow-memory-netty is just not supported when using modules because of the patching it does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1826967158
> I'm not sure what's happening in this CI failure after the Netty refactoring.
> This test was in TestBaseAllocator before, but that test class got moved to memory-core. This test got separated into its own test class, moved back to Netty core (because it has Netty dependencies and wouldn't build in memory-core), and now throws a ConcurrentModificationException.
>
> > Error: org.apache.arrow.memory.netty.TestNettyAllocator.testMemoryUsage Time elapsed: 0.317 s <<< ERROR!
> java.util.ConcurrentModificationException
> at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1604)
> at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
> at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
> at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
> at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
> at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
> at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
> at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
> at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:632)
> at org.apache.arrow.memory.netty.TestNettyAllocator.testMemoryUsage(TestNettyAllocator.java:56)
This didn't happen in my last test run and all I did was change the module info computer plugin to inherit its version. So this is probably a threading issue. Maybe extracting this test from other TestBaseAllocator tests is causing them to run in parallel when they weren't before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1825767470
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1825967068
This failure is happening for somewhat complicated reasons:
https://github.com/apache/arrow/actions/runs/6983421948/job/19004513065?pr=38876#step:6:2511
Before refactoring, the allocator used by this test would be the Netty module's DefaultAllocationManagerFactory since TestBaseAllocator lived in the Netty module. This one correctly returns a non-zero value when given a zero-allocation.
After refactoring, TestBaseAllocator lives in memory-core, and uses the dummy DefaultAllocationManagerFactory in memory-core's tests. This one however does return zero when given a zero-allocation. This might itself be a bug unrelated to the refactoring. This calls MemoryUtil.UNSAFE.allocateMemory(), which calls sun.misc.Unsafe.allocateMemory(), which is supposed to return a non-zero value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829326776
:warning: GitHub issue #38915 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829707759
These changes are requiring alot of patching/add-opens calls to work (see the changes It is very unfriendly for the user.
https://github.com/apache/arrow/pull/38876/files#diff-d8225ebcfc11b480a6e4f54e183b67c3ead51635a167c106d928c2abf1f9ef66R892
The patch-module call is particularly difficult because we, and the user, do not necessarily know where their JARs are.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1828899964
I'm looking for some ideas on how to fix up a maven issue. I've rigged the general build configuration in the parent POM to continue to use JDK 8 and exclude module-info.java files. I've also shut off using the module path for dependencies.
In the memory-netty build, I need a profile (that runs on JDK9) that compiles using the module-path for dependencies (using the <useModulePath> flag added to maven-compiler-plugin 3.11.0). It needs to run an extra command to patch code from another JAR into some of the packages that Netty modules export. I'd also like to compile module-info.java for this.
I likely need two executions for the Netty build. One that compiles everything in JDK8 except module-info.java, and one that uses JDK 9, compiles module-info.java and does the patch command. I don't want to compile anything else with JDK9 since I need to generate JDK8 bytecode for everything else (or I can compile everything in JDK9, then recompile everything except module-info.java in 8)
The problem I'm seeing is that setting the flag for setting the <useModulePath> flag in the profile's configuration isn't taking effect. This problem happens when both when it's an execution-level configuration or a plugin-level configuration. It does take effect when set on the parent POM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1825772431
This is roughly comparable to where #13072 got to, minus the actual module-info.java files.
With the following additions:
- Built on the current state of main
- Updated gRPC to a module-friendly version (1.59).
- Updated plugin dependencies to module-friendly versions.
- Staged to compile the codebase with Java 8, with the exception of module-info files which will use a custom plugin (to be added. this is already implemented at https://github.com/Bit-Quill/maven-module-info-compiler-plugin and needs to get integrated to the Arrow build).
@davisusanibar @lidavidm @danepitkin
Next up will be adding the maven plugin, then adding the module-info files from #13072, then modularizing other modules such as algorithm, adapters, ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1828277417
> > This failure is happening for somewhat complicated reasons: https://github.com/apache/arrow/actions/runs/6983421948/job/19004513065?pr=38876#step:6:2511
> > Before refactoring, the allocator used by this test would be the Netty module's DefaultAllocationManagerFactory since TestBaseAllocator lived in the Netty module. This one correctly returns a non-zero value when given a zero-allocation.
> > After refactoring, TestBaseAllocator lives in memory-core, and uses the dummy DefaultAllocationManagerFactory in memory-core's tests. This one however does return zero when given a zero-allocation. This might itself be a bug unrelated to the refactoring. This calls MemoryUtil.UNSAFE.allocateMemory(), which calls sun.misc.Unsafe.allocateMemory(), which is supposed to return a non-zero value.
> > I'm going to @ignore this test for now.
> > Should these allocation manager factorys return a non-zero address when the user asks for an empty allocation? I would assume so since C-allocators have this property.
> > @davisusanibar , did you see anything like this? @lidavidm @danepitkin , any thoughts around this?
>
> Thanks for pushing this feature, James. I am observing changes. Does this problem persist?
Yes this still happens. I have marked the test as ignored to get past it.
It's not clear if this is a new bug or if it is exposing a bug from the memory-core test version of DefaultAllocationManagerFactory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1836535199
This PR has been split into several smaller PRs and issues.
See #38915 for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1826808330
I'm not sure what's happening in this CI failure after the Netty refactoring.
This test was in TestBaseAllocator before, but that test class got moved to memory-core. This test got separated into its own test class, moved back to Netty core (because it has Netty dependencies and wouldn't build in memory-core), and now throws a ConcurrentModificationException.
> Error: org.apache.arrow.memory.netty.TestNettyAllocator.testMemoryUsage Time elapsed: 0.317 s <<< ERROR!
java.util.ConcurrentModificationException
at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1604)
at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:632)
at org.apache.arrow.memory.netty.TestNettyAllocator.testMemoryUsage(TestNettyAllocator.java:56)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829858143
Given how new this is, I would even be OK saying that the only supported memory implementation is the Java 21+ one to begin with and we can see if there is even demand to use memory-unsafe or memory-netty.
Finally, if there's need to reorganize which package files live in, maybe we could split some of that into another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829703021
The test errors in memory-core have this cause:
`Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to module org.apache.arrow.memory.core`
I've addressed this is in maven by adding --add-opens java.base/java.nio=org.apache.arrow.memory.core, but perhaps I haven't covered a profile that CI is using. If anyone has any ideas on what else might need to be edited that'd be really helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1828222610
> This failure is happening for somewhat complicated reasons: https://github.com/apache/arrow/actions/runs/6983421948/job/19004513065?pr=38876#step:6:2511
>
> Before refactoring, the allocator used by this test would be the Netty module's DefaultAllocationManagerFactory since TestBaseAllocator lived in the Netty module. This one correctly returns a non-zero value when given a zero-allocation.
>
> After refactoring, TestBaseAllocator lives in memory-core, and uses the dummy DefaultAllocationManagerFactory in memory-core's tests. This one however does return zero when given a zero-allocation. This might itself be a bug unrelated to the refactoring. This calls MemoryUtil.UNSAFE.allocateMemory(), which calls sun.misc.Unsafe.allocateMemory(), which is supposed to return a non-zero value.
>
> I'm going to @ignore this test for now.
>
> Should these allocation manager factorys return a non-zero address when the user asks for an empty allocation? I would assume so since C-allocators have this property.
>
> @davisusanibar , did you see anything like this? @lidavidm @danepitkin , any thoughts around this?
Thanks for pushing this feature, James. I am observing changes. Does this problem persist?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1829704392
This PR has alot of changes. Perhaps we handle other modules (such as Flight, algorithm, dataset) in a separate PR so that this one can close.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1825884907
This suggests that we need to install JDK 9+ on some of the build agents:
https://github.com/apache/arrow/actions/runs/6982068919/job/19000467907?pr=38876#step:6:791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38915: [Java] Upgrade Arrow Java project to JPMS Java Platform Module System [arrow]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #38876:
URL: https://github.com/apache/arrow/pull/38876#issuecomment-1832653894
> Only looking at the memory changes, they seem reasonable.
>
> Is there a way to get rid of the classes declared in Netty's namespace? Or is what we're trying to do still not possible with just the public APIs?
The classes in the Netty namespace need to stay there. They call package-private functions on Netty classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org