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

DRILL-8467: Update Netty to 4.1.101 #2857

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Conversation

rymarm
Copy link
Member

@rymarm rymarm commented Dec 13, 2023

DRILL-8467: Update netty to 4.1.101

Description

Update Netty to 4.1.101.
Netty 4.1.75 has 2 "breaking" changes:

  1. The default PooledByteBufAllocator chunk size was reduced from 16 MiB to 4 MiB
  2. The default value of io.netty.allocator.useCacheForAllThreads was changed to false

Source: https://netty.io/news/2022/03/10/4-1-75-Final.html

I made InnerAllocator creation with the previously accepted chunk size for Drill - 16 MiB, and left useCacheForAllThreads enabled because as far as I understand cache for all threads(not only for the Netty theads) gives better performance for Drill case.

I left those options overridable by Netty Java properties io.netty.allocator.useCacheForAllThreads and io.netty.allocator.maxOrder(responds for chunk size), so it can be changed without Drill recompilation as it was before.

Documentation

No changes

Testing

Run unit tests.

@cgivre cgivre added doc-impacting PRs that affect the documentation security dependencies labels Dec 17, 2023
@cgivre
Copy link
Contributor

cgivre commented Dec 17, 2023

@rymarm Would it make sense to add config options for these "new" parameters?

@rymarm
Copy link
Member Author

rymarm commented Dec 18, 2023

@cgivre I don't think that we need to add config options for changing the default chunk size and allocation cache enabling, because both are fundamental for Drill and I don't think there are unique cases when Drill works better with other than default values so a user wishes to change them.

But even If a user wishes to change them for some reason, he still can use java options -Dio.netty.allocator.useCacheForAllThreads and -Dio.netty.allocator.maxOrder to change the values.

@rymarm
Copy link
Member Author

rymarm commented Dec 18, 2023

I need to find out now, why Drill tests on Java 8 failed with our of memory error:

Errors:   
[ERROR]   TestValueVector.testFixedVectorReallocation »  Unexpected exception, expected<org.apache.drill.exec.exception.OversizedAllocationException> but was<org.apache.drill.exec.exception.OutOfMemoryException>   
[ERROR]   TestValueVector.testVariableVectorReallocation »  Unexpected exception, expected<org.apache.drill.exec.exception.OversizedAllocationException> but was<org.apache.drill.exec.exception.OutOfMemoryException>

@jnturton
Copy link
Contributor

Hi @rymarm! Here are two things it would be interesting to redo the Java 8 runs with.

  1. Adding the EasyOutOfMemory annotation to TestValueVector.java.
  2. Bumping the directMemoryMb value in pom.xml. We set up a swap file at the start of the CI run now so you could try to taking this up to 3Gb.

@jnturton jnturton changed the title Update Netty to 4.1.101 DRILL-8467: Update Netty to 4.1.101 Dec 31, 2023
@jnturton
Copy link
Contributor

jnturton commented Dec 31, 2023

P.S. the failing test class runs fine in isolation on my laptop under IBM JDK 8 so I do think we can fix this in the CI with some environment tweaks.

@jnturton
Copy link
Contributor

@rymarm I just got the same test failure in the 1.21 branch which is still based on Netty 4.1.73.Final. I'm testing there with more direct memory and will send a PR to master if that takes care of the issue.

@jnturton jnturton added the backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there label Dec 31, 2023
@jnturton
Copy link
Contributor

Update on my comments above: the direct memory limit increase has just been merged to master. Please rebase this and let's see what we get.

@rymarm
Copy link
Member Author

rymarm commented Dec 31, 2023

@jnturton Thank you so much for the investigation! I didn't have time to check tests failures.
P.S. You decide to finish all unresolved tasks until the end of the year?)

@jnturton
Copy link
Contributor

P.S. You decide to finish all unresolved tasks until the end of the year?)

😂

@jnturton
Copy link
Contributor

We just need to bump up the JAR size limit in the hadoop-2 profile at the bottom of the pom in jdbc-all now.

Copy link
Contributor

@jnturton jnturton left a comment

Choose a reason for hiding this comment

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

Awesome work, the 13 line diff hides the trickiness of this upgrade.

@jnturton jnturton merged commit 0f71146 into apache:master Jan 2, 2024
8 checks passed
jnturton pushed a commit to jnturton/drill that referenced this pull request Jan 3, 2024
jnturton pushed a commit to jnturton/drill that referenced this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there dependencies doc-impacting PRs that affect the documentation security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants