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/17 14:01:14 UTC
[GitHub] [pulsar] kijanowski opened a new pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
kijanowski opened a new pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381
### Motivation
`pulsar-client` runs `conf/pulsar_tools_env.sh` to set `PULSAR_EXTRA_OPTS`. If `PULSAR_EXTRA_OPTS` are given in the command line and contain `-Xmx`, it will be overridden by `PULSAR_MEM`.
### Modifications
Exchange the order of applying env variables. First go the hardcoded PULSAR_MEM settings, then potentially the user settings.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
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`
Change in a shell script, which is not documented.
--
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] kijanowski commented on a change in pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
kijanowski commented on a change in pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381#discussion_r771443082
##########
File path: conf/pulsar_tools_env.sh
##########
@@ -48,7 +48,7 @@ PULSAR_MEM="-Xmx128m -XX:MaxDirectMemorySize=128m"
PULSAR_GC=" -client "
# Extra options to be passed to the jvm
-PULSAR_EXTRA_OPTS="${PULSAR_EXTRA_OPTS} ${PULSAR_MEM} ${PULSAR_GC} ${PULSAR_GC_LOG} -Dio.netty.leakDetectionLevel=disabled"
+PULSAR_EXTRA_OPTS="${PULSAR_MEM} ${PULSAR_EXTRA_OPTS} ${PULSAR_GC} ${PULSAR_GC_LOG} -Dio.netty.leakDetectionLevel=disabled"
Review comment:
Thank you, done
--
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 a change in pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381#discussion_r771422679
##########
File path: conf/pulsar_tools_env.sh
##########
@@ -48,7 +48,7 @@ PULSAR_MEM="-Xmx128m -XX:MaxDirectMemorySize=128m"
PULSAR_GC=" -client "
# Extra options to be passed to the jvm
-PULSAR_EXTRA_OPTS="${PULSAR_EXTRA_OPTS} ${PULSAR_MEM} ${PULSAR_GC} ${PULSAR_GC_LOG} -Dio.netty.leakDetectionLevel=disabled"
+PULSAR_EXTRA_OPTS="${PULSAR_MEM} ${PULSAR_EXTRA_OPTS} ${PULSAR_GC} ${PULSAR_GC_LOG} -Dio.netty.leakDetectionLevel=disabled"
Review comment:
+1. One slight improvement:
Please move `${PULSAR_EXTRA_OPTS}` as the very last entry so that it will also be possible to override `io.netty.leakDetectionLevel` when needed.
btw. I happened to do a similar change in `bin/pulsar` script recently in https://github.com/apache/pulsar/pull/13025/files#diff-89618c73d511b2015766c1fb0cd4bd16aa7f949dfdfa02d1995b586b8ed57d7aL283
--
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] Shoothzj commented on pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381#issuecomment-997175738
Why we need this feature, you can config both `PULSAR_MEM` and `PULSAR_EXTRA_OPS`
cc @lhotari @michaeljmarshall
--
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 #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381#issuecomment-997191880
> Why we need this feature, you can config both `PULSAR_MEM` and `PULSAR_EXTRA_OPS` cc @lhotari @michaeljmarshall
I believe that this change improves consistency. The assumption is that you can override other options by setting them in PULSAR_EXTRA_OPTS. I made a similar improvement to consistency for `bin/pulsar` as part of PR #13025 .
Another point is that this change isn't harmful or risky.
--
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 edited a comment on pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381#issuecomment-997191880
> Why we need this feature, you can config both `PULSAR_MEM` and `PULSAR_EXTRA_OPS` cc @lhotari @michaeljmarshall
@Shoothzj I believe that this change improves consistency. The assumption is that you can override other options by setting them in PULSAR_EXTRA_OPTS. I made a similar improvement to consistency for `bin/pulsar` as part of PR #13025 .
Another point is that this change isn't harmful or risky.
--
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] codelipenghui merged pull request #13381: [Client]Allow to override PULSAR_MEM settings via PULSAR_EXTRA_OPS
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #13381:
URL: https://github.com/apache/pulsar/pull/13381
--
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