[Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x by HQebupt · Pull Request #13563 · apache/pulsar

HQebupt · 2021-12-29T16:06:37Z

Motivation

The configuration io.netty.recycler.maxCapacity.default does not work in Netty 4.1.x. And The Recycler class should use io.netty.recycler.maxCapacityPerThread or io.netty.recycler.maxCapacity as maxCapacityPerThread configuration in Netty 4.1.x.

Reference Netty code: https://github.com/netty/netty/blob/d5faba2fbeb7da65daa0a10f41560f47443c1026/common/src/main/java/io/netty/util/Recycler.java#L58

Modifications

Change io.netty.recycler.maxCapacity.default to io.netty.recycler.maxCapacityPerThread in start up script.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

no-need-doc

eolivelli

This is interesting.

Do you have some numbers about the impact of this change?

As this is broken and current property is not taking effect...
Why not removing the property at all and let users configure it?

I am not sure about the actual benefit or the impact of restoring that option

@merlimat @MMirelli @dave2wave

HQebupt · 2021-12-30T02:25:09Z

This is interesting.

Do you have some numbers about the impact of this change?

As this is broken and current property is not taking effect... Why not removing the property at all and let users configure it?

I am not sure about the actual benefit or the impact of restoring that option

@merlimat @MMirelli @dave2wave

Hi Enrico, I did not see any performance impact. I agree with you. And we can use the default value (io.netty.recycler.maxCapacityPerThread=4096) explicitly in that it keeps the same configuration as before in Pulsar. How about it?

HQebupt · 2021-12-31T11:20:33Z

/pulsarbot run-failure-checks

HQebupt · 2022-01-18T09:31:26Z

Hi @eolivelli @merlimat @MMirelli @dave2wave , what are your thoughts ? Looking forward your response.

HQebupt · 2022-01-29T08:47:57Z

@codelipenghui @hangc0276 @Jason918 @shoothzj PTAL

Jason918

LGTM

HQebupt · 2022-02-08T02:22:09Z

@eolivelli Hi Enrico, Please take a look the PR, thanks in advance.

MMirelli · 2022-02-08T08:26:19Z

I will check whether this may cause evident perf degradation within this week, hopefully. Thank you for pointing this out.

michaeljmarshall

Hi Enrico, I did not see any performance impact. I agree with you. And we can use the default value (io.netty.recycler.maxCapacityPerThread=4096) explicitly in that it keeps the same configuration as before in Pulsar. How about it?

@HQebupt - can you explain why 4096 is the same configuration as before? It seems like 1000 was the configuration before.

HQebupt · 2022-02-09T03:36:52Z

Hi Enrico, I did not see any performance impact. I agree with you. And we can use the default value (io.netty.recycler.maxCapacityPerThread=4096) explicitly in that it keeps the same configuration as before in Pulsar. How about it?

@HQebupt - can you explain why 4096 is the same configuration as before? It seems like 1000 was the configuration before.
Please see https://github.com/netty/netty/blob/d5faba2fbeb7da65daa0a10f41560f47443c1026/common/src/main/java/io/netty/util/Recycler.java#L49

HQebupt · 2022-02-12T13:56:59Z

@michaeljmarshall @315157973 PTAL

michaeljmarshall

LGTM.

@HQebupt - thanks for the reference. Do you know which versions of Pulsar need this patch?

HQebupt · 2022-03-07T11:57:50Z

The next version, maybe 2.11.0 :)

HQebupt · 2022-03-07T12:00:06Z

@codelipenghui Hi penghui, could you please merge this pr since three committers approved it already ? thanks in advance.

lhotari

Please replace all occurences of -Dio.netty.recycler.maxCapacity.default=1000 with -Dio.netty.recycler.maxCapacityPerThread=4096 in all source files (including documentation).

HQebupt · 2022-03-08T12:46:14Z

@lhotari @hangc0276 @eolivelli @315157973 PTAL again. Thanks

lhotari

The io.netty.recycler.linkCapacity system property was removed in Netty 4.1.71.Final
by commit netty/netty@40196a63 . Please also remove all references to io.netty.recycler.linkCapacity.

HQebupt · 2022-03-18T06:19:27Z

The io.netty.recycler.linkCapacity system property was removed in Netty 4.1.71.Final
by commit netty/netty@40196a63 . Please also remove all references to io.netty.recycler.linkCapacity.

@lhotari Apology for delay fix. PTAL. Thanks

HQebupt · 2022-04-01T07:01:52Z

@lhotari PTAL. Thank you.

HQebupt · 2022-04-02T03:59:22Z

@lhotari PTAL. Thank you.

HQebupt · 2022-04-02T04:00:24Z

/pulsarbot run-failure-checks

…4.1.x

HQebupt · 2022-04-11T03:05:31Z

/pulsarbot run-failure-checks

…4.1.x (apache#13563)

…4.1.x (#13563) (cherry picked from commit 4721ce1)

…4.1.x (apache#13563) (cherry picked from commit 4721ce1) (cherry picked from commit 5206262)

hangc0276 · 2023-02-07T03:55:42Z

This PR is tagged with 2.8.5, when we cherry-pick this PR into branch-2.8, we also need to cherry-pick #19441, otherwise, the bookie couldn't apply the memory and GC configurations.

github-actions Bot assigned HQebupt Dec 29, 2021

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 29, 2021

eolivelli reviewed Dec 29, 2021

View reviewed changes

HQebupt force-pushed the nettyOps branch from d4b9faa to 80e2326 Compare December 30, 2021 08:47

HQebupt mentioned this pull request Jan 15, 2022

invalid config item -Dio.netty.recycler.maxCapacity.default #13757

Closed

HQebupt requested a review from eolivelli January 16, 2022 06:59

Jason918 approved these changes Jan 29, 2022

View reviewed changes

Jason918 requested review from 315157973, codelipenghui, hangc0276, lhotari and michaeljmarshall February 8, 2022 04:05

codelipenghui approved these changes Feb 8, 2022

View reviewed changes

315157973 reviewed Feb 8, 2022

View reviewed changes

Comment thread conf/bkenv.sh Outdated

michaeljmarshall reviewed Feb 8, 2022

View reviewed changes

HQebupt requested review from 315157973 and michaeljmarshall February 12, 2022 13:56

michaeljmarshall approved these changes Feb 15, 2022

View reviewed changes

michaeljmarshall mentioned this pull request Mar 7, 2022

Remove invalid Netty system property io.netty.recycler.maxCapacity.default which was renamed in 2014 #14576

Closed

lhotari requested changes Mar 8, 2022

View reviewed changes

michaeljmarshall approved these changes Mar 9, 2022

View reviewed changes

lhotari requested changes Mar 9, 2022

View reviewed changes

HQebupt force-pushed the nettyOps branch from 771238d to a214ee8 Compare March 18, 2022 06:16

HQebupt requested a review from lhotari March 18, 2022 06:31

Remove invalid Netty system property which never was valid for Netty …

f98ca3c

…4.1.x

HQebupt force-pushed the nettyOps branch from a214ee8 to f98ca3c Compare April 10, 2022 09:15

lhotari approved these changes Apr 12, 2022

View reviewed changes

lhotari merged commit 4721ce1 into apache:master Apr 12, 2022

aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Apr 18, 2022

Remove invalid Netty system property which never was valid for Netty …

bc8de3a

…4.1.x (apache#13563)

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

Remove invalid Netty system property which never was valid for Netty …

8100fe9

…4.1.x (apache#13563)

lhotari added release/2.9.4 release/2.8.5 release/2.11.1 release/2.10.3 labels Oct 5, 2022

congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 4, 2022

congbobo184 pushed a commit that referenced this pull request Nov 30, 2022

Remove invalid Netty system property which never was valid for Netty …

1fa9e1e

…4.1.x (#13563) (cherry picked from commit 4721ce1)

liangyepianzhou pushed a commit that referenced this pull request Dec 6, 2022

Remove invalid Netty system property which never was valid for Netty …

5206262

…4.1.x (#13563) (cherry picked from commit 4721ce1)

liangyepianzhou added the cherry-picked/branch-2.10 label Dec 6, 2022

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 6, 2022

Remove invalid Netty system property which never was valid for Netty …

ea98d5a

…4.1.x (apache#13563) (cherry picked from commit 4721ce1) (cherry picked from commit 5206262)

hangc0276 added release/2.8.5 and removed release/2.8.5 labels Feb 7, 2023

hangc0276 mentioned this pull request Feb 7, 2023

[fix][Branch-2.9] Fix bookie JVM param BOOKIE_MEM, BOOKIE_GC miss problem. #19441

Merged

15 tasks

Technoboy- added the cherry-picked/branch-2.11 label Feb 7, 2023

Uh oh!

Conversation

HQebupt commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

eolivelli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Dec 30, 2021

Uh oh!

HQebupt commented Dec 31, 2021

Uh oh!

HQebupt commented Jan 18, 2022

Uh oh!

HQebupt commented Jan 29, 2022

Uh oh!

Jason918 left a comment

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Feb 8, 2022

Uh oh!

MMirelli commented Feb 8, 2022

Uh oh!

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Feb 9, 2022

Uh oh!

HQebupt commented Feb 12, 2022

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Mar 7, 2022

Uh oh!

HQebupt commented Mar 7, 2022

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Mar 8, 2022

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

HQebupt commented Mar 18, 2022

Uh oh!

HQebupt commented Apr 1, 2022

Uh oh!

HQebupt commented Apr 2, 2022

Uh oh!

HQebupt commented Apr 2, 2022

Uh oh!

HQebupt commented Apr 11, 2022

Uh oh!

hangc0276 commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

HQebupt commented Dec 29, 2021 •

edited

Loading

eolivelli left a comment •

edited

Loading