You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/29 16:06:45 UTC
[GitHub] [pulsar] HQebupt opened a new pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
HQebupt opened a new pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563
### 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
- [x] 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?
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1033310882
> > 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
<img width="1048" alt="image" src="https://user-images.githubusercontent.com/4970972/153117142-998dbafb-ea66-4902-98e2-59ad96a5ab2a.png">
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] MMirelli commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
MMirelli commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1032338223
I will check whether this may cause evident perf degradation within this week, hopefully. Thank you for pointing this out.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1060611934
@codelipenghui Hi penghui, could you please merge this pr since three committers approved it already ? thanks in advance.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#discussion_r801770166
##########
File path: conf/bkenv.sh
##########
@@ -48,7 +48,7 @@ else
fi
# Extra options to be passed to the jvm
-BOOKIE_EXTRA_OPTS="${BOOKIE_EXTRA_OPTS:-"-Dio.netty.leakDetectionLevel=disabled ${PULSAR_EXTRA_OPTS:-"-Dio.netty.recycler.maxCapacity.default=1000 -Dio.netty.recycler.linkCapacity=1024"}"}"
+BOOKIE_EXTRA_OPTS="${BOOKIE_EXTRA_OPTS:-"-Dio.netty.leakDetectionLevel=disabled ${PULSAR_EXTRA_OPTS:-"-Dio.netty.recycler.maxCapacityPerThread=4096 -Dio.netty.recycler.linkCapacity=1024"}"}"
Review comment:
So, why we change the default value from 1000 to 4096 ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1037232999
@michaeljmarshall @315157973 PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1061700445
> > LGTM.
> > @HQebupt - thanks for the reference. Do you know which versions of Pulsar need this patch?
>
> @michaeljmarshall This applies to all Pulsar versions since v1.21.0-incubating. The `-Dio.netty.recycler.maxCapacity.default=1000` setting stopped working after Netty 4.1 upgrade in #689 ([v1.21.0-incubating](https://github.com/apache/pulsar/releases/tag/v1.21.0-incubating)). The effective setting has been `-Dio.netty.recycler.maxCapacityPerThread=4096` so that's why it makes sense to either remove the setting completely (as suggested in #14576) or replace all occurrences of `-Dio.netty.recycler.maxCapacity.default=1000` with `-Dio.netty.recycler.maxCapacityPerThread=4096` in all source files (including documentation).
Sure, let me do it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1061740271
@lhotari @hangc0276 @eolivelli @315157973 PTAL again. Thanks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1086538893
@lhotari PTAL. Thank you.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1085506691
@lhotari PTAL. Thank you.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1003350211
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1015226643
Hi @eolivelli @merlimat @MMirelli @dave2wave , what are your thoughts ? Looking forward your response.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1072067380
> The `io.netty.recycler.linkCapacity` system property was removed in Netty 4.1.71.Final
> by commit [netty/netty@40196a63](https://github.com/netty/netty/commit/40196a63) . Please also remove all references to `io.netty.recycler.linkCapacity`.
@lhotari Apology for delay fix. PTAL. Thanks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1086539097
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a change in pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on a change in pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#discussion_r802241948
##########
File path: conf/bkenv.sh
##########
@@ -48,7 +48,7 @@ else
fi
# Extra options to be passed to the jvm
-BOOKIE_EXTRA_OPTS="${BOOKIE_EXTRA_OPTS:-"-Dio.netty.leakDetectionLevel=disabled ${PULSAR_EXTRA_OPTS:-"-Dio.netty.recycler.maxCapacity.default=1000 -Dio.netty.recycler.linkCapacity=1024"}"}"
+BOOKIE_EXTRA_OPTS="${BOOKIE_EXTRA_OPTS:-"-Dio.netty.leakDetectionLevel=disabled ${PULSAR_EXTRA_OPTS:-"-Dio.netty.recycler.maxCapacityPerThread=4096 -Dio.netty.recycler.linkCapacity=1024"}"}"
Review comment:
@michaeljmarshall @315157973 The `io.netty.recycler.maxCapacity.default=1000` is invalid. It means the configuration does not work. And by default `io.netty.recycler.maxCapacityPerThread` is 4096. I explicitly point out here. Please refer https://github.com/netty/netty/blob/d5faba2fbeb7da65daa0a10f41560f47443c1026/common/src/main/java/io/netty/util/Recycler.java#L49
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1037232999
@michaeljmarshall @315157973 PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1060610365
>
The next version, maybe 2.11.0 :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1032145081
@eolivelli Hi Enrico, Please take a look the PR, thanks in advance.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1002845526
> 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?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1072067380
> The `io.netty.recycler.linkCapacity` system property was removed in Netty 4.1.71.Final
> by commit [netty/netty@40196a63](https://github.com/netty/netty/commit/40196a63) . Please also remove all references to `io.netty.recycler.linkCapacity`.
@lhotari Apology for delay fix. PTAL. Thanks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1061422621
> LGTM.
>
> @HQebupt - thanks for the reference. Do you know which versions of Pulsar need this patch?
@michaeljmarshall This applies to all Pulsar versions since v1.21.0-incubating. The `-Dio.netty.recycler.maxCapacity.default=1000` setting stopped working after Netty 4.1 upgrade in #689 ([v1.21.0-incubating](https://github.com/apache/pulsar/releases/tag/v1.21.0-incubating)). The effective setting has been `-Dio.netty.recycler.maxCapacityPerThread=4096` so that's why it makes sense to either remove the setting completely (as suggested in #14576) or replace all occurrences of `-Dio.netty.recycler.maxCapacity.default=1000` with `-Dio.netty.recycler.maxCapacityPerThread=4096` in all source files (including documentation).
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #13563: [Improvement] Recycler should use io.netty.recycler.maxCapacityPerThread instead of io.netty.recycler.maxCapacity.default as maxCapacityPerThread configuration in Netty 4.1.x
Posted by GitBox <gi...@apache.org>.
HQebupt commented on pull request #13563:
URL: https://github.com/apache/pulsar/pull/13563#issuecomment-1024868947
@codelipenghui @hangc0276 @Jason918 @Shoothzj PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org