Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Java7+ NIO API for improved performance #5350

Merged
merged 7 commits into from
Aug 15, 2023

Conversation

mkarg
Copy link
Member

@mkarg mkarg commented Jun 18, 2023

This PR is a partial backport of #5341 as proposed by @jansupol in #5341 (comment).

Compared to PR 5341 it only contains those changes which are compatible with Java 8. In particular this means, it mostly consists of using Java 7+ NIO API plus some more improvements, while the actual benefit (preventing data transmission via the JVM's RAM implemented by ChannelInputStream::transferTo in recent JREs) is missing due to the fact that InputStream::transferTo was not added to the JRE before Java 9.

@mkarg mkarg force-pushed the nio-master branch 2 times, most recently from 83caa92 to b69481a Compare June 18, 2023 11:37
@mkarg mkarg changed the title Using Java7+ NIO API for improved performance: Modern JREs offload to… Using Java7+ NIO API for improved performance Jun 18, 2023
@mkarg mkarg marked this pull request as ready for review June 18, 2023 13:12
@mkarg
Copy link
Member Author

mkarg commented Jul 6, 2023

@jansupol You asked me to provide these changes explicitly for the master branch, so why not merging it? The PR is open since three weeks, and it solely contains backports of the already reviewed original PR from the 3.x branch.

@mkarg
Copy link
Member Author

mkarg commented Jul 14, 2023

@jansupol I think the communication between Github and Jenkins is broken. I fixed another missing import last night and now Github says it is waiting for the build, but Jenkins says no builds are queued. Is there a way to manually trigger the build?

@senivam
Copy link
Contributor

senivam commented Jul 14, 2023

@mkarg, the build is triggered manually, but it does not pass compilation on JDK 1.8 (that thread has already failed).

@mkarg
Copy link
Member Author

mkarg commented Jul 14, 2023

@senivam Thank you for triggering the build. Let's wait for it to finish. If there are still bugs in the PR I will fix them ASAP once reported.

Edit: Ah, I see. The @forRemoval didn't exist on Java 8. Backporting the original PR is more tricky than expected. ;-)

@mkarg
Copy link
Member Author

mkarg commented Jul 14, 2023

@senivam Maxim, it seems there are no more Java problems in this PR other than the ones produced by Github/Jenkins themselves. In particular, I assume this is unrelated to my work:

Creating placeholder flownodes because failed loading originals.
ERROR: Cannot resume build because FlowNode 41 for FlowHead 1 could not be loaded. This is expected to happen when using the PERFORMANCE_OPTIMIZED durability setting and Jenkins is not shut down cleanly. Consider investigating to understand if Jenkins was not shut down cleanly or switching to the MAX_SURVIVABILITY durability setting which should prevent this issue in most cases.

GitHub has been notified of this commit’s build result

Finished: FAILURE

@senivam
Copy link
Contributor

senivam commented Jul 17, 2023

I've run the build again, the previous attempt even did not start properly (it's kind of known Eclipse's CI issue).

@mkarg
Copy link
Member Author

mkarg commented Jul 17, 2023

I've run the build again, the previous attempt even did not start properly (it's kind of known Eclipse's CI issue).

Thank you. Meanwhile your manually triggered build terminated successfully, so this PR is (at last!) ready to merge. 🥳

@mkarg
Copy link
Member Author

mkarg commented Aug 2, 2023

@jansupol Is there something else you want me to fix in this PR? I have not seen open comments, and the last status update is two three weeks ago. Did I miss to fix something?

@mkarg
Copy link
Member Author

mkarg commented Aug 11, 2023

@senivam Maxim I wonder why there is no progress with this PR. It fulfils all changes requested by Jan since weeks. Jan did not answer, so maybe you could share some thoughts what I need to do to finally get this PR merged? Thanks a lot! :-)

Copy link
Contributor

@jansupol jansupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkarg No need to do anything, this looks good. Thanks.

Copy link
Contributor

@senivam senivam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@senivam senivam merged commit 81d03fc into eclipse-ee4j:2.x Aug 15, 2023
@senivam senivam added this to the 2.41 milestone Aug 15, 2023
@mkarg mkarg deleted the nio-master branch August 15, 2023 17:50
srowen pushed a commit to apache/spark that referenced this pull request Oct 29, 2023
### What changes were proposed in this pull request?
This pr aims upgrade Jersey from 2.40 to 2.41.

### Why are the changes needed?
The new version bring some improvements, like:
- eclipse-ee4j/jersey#5350
- eclipse-ee4j/jersey#5365
- eclipse-ee4j/jersey#5436
- eclipse-ee4j/jersey#5296

and some bug fix, like:
- eclipse-ee4j/jersey#5359
- eclipse-ee4j/jersey#5405
- eclipse-ee4j/jersey#5423
- eclipse-ee4j/jersey#5435
- eclipse-ee4j/jersey#5445

The full release notes as follows:
- https://github.com/eclipse-ee4j/jersey/releases/tag/2.41

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43490 from LuciferYang/SPARK-45636.

Lead-authored-by: YangJie <yangjie01@baidu.com>
Co-authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants