cckellogg commented on a change in pull request #5790: [Issue:5687] Add prefix
for new keys from Env
URL: https://github.com/apache/pulsar/pull/5790#discussion_r353257885
##########
File path: deployment/kubernetes/aws/broker.yaml
##########
@@ -25,15 +25,15 @@ metadata:
data:
# Tune for available memory. Increase the heap up to 24G to have
# better GC behavior at high throughput
- PULSAR_MEM: "\" -Dio.netty.leakDetectionLevel=disabled
-Dio.netty.recycler.linkCapacity=1024 -XX:+ParallelRefProcEnabled
-XX:+UnlockExperimentalVMOptions -XX:+AggressiveOpts -XX:+DoEscapeAnalysis
-XX:ParallelGCThreads=32 -XX:ConcGCThreads=32 -XX:G1NewSizePercent=50
-XX:+DisableExplicitGC -XX:-ResizePLAB -XX:+ExitOnOutOfMemoryError
-XX:+PerfDisableSharedMem -Xms12g -Xmx12g -XX:MaxDirectMemorySize=14g
-Dpulsar.root.logger=DEBUG,FILE \""
- PULSAR_GC: "\" -XX:+UseG1GC -XX:MaxGCPauseMillis=10\""
- zookeeperServers: zk-0.zookeeper,zk-1.zookeeper,zk-2.zookeeper
- configurationStoreServers: zk-0.zookeeper,zk-1.zookeeper,zk-2.zookeeper
- clusterName: us-east
- managedLedgerDefaultEnsembleSize: "2"
- managedLedgerDefaultWriteQuorum: "2"
- managedLedgerDefaultAckQuorum: "2"
- deduplicationEnabled: "false"
+ PULSAR_PREFIX_PULSAR_MEM: "\" -Dio.netty.leakDetectionLevel=disabled
-Dio.netty.recycler.linkCapacity=1024 -XX:+ParallelRefProcEnabled
-XX:+UnlockExperimentalVMOptions -XX:+AggressiveOpts -XX:+DoEscapeAnalysis
-XX:ParallelGCThreads=32 -XX:ConcGCThreads=32 -XX:G1NewSizePercent=50
-XX:+DisableExplicitGC -XX:-ResizePLAB -XX:+ExitOnOutOfMemoryError
-XX:+PerfDisableSharedMem -Xms12g -Xmx12g -XX:MaxDirectMemorySize=14g
-Dpulsar.root.logger=DEBUG,FILE \""
Review comment:
There is no need to put a prefix in front of PULSAR_MEM, PULSAR_GC, or
PULSAR_EXTRA_OPTS. This script
(https://github.com/apache/pulsar/blob/master/conf/pulsar_env.sh) will pick up
the environment variables. Same for all the other places.
Also, adding the PULSAR_PREFIX everywhere will add random variables to the
configs. For example the broker is started with the following command
```
bin/apply-config-from-env.py conf/broker.conf &&
bin/apply-config-from-env.py conf/pulsar_env.sh &&
bin/gen-yml-from-env.py conf/functions_worker.yml &&
bin/pulsar broker
```
Every PULSAR_PREFX_ key value will end up in the conf/pulsar_env.sh script.
That might not matter now or break things but I don't think is the correct
approach in general for the broker or proxy configs. In my opinion the override
scripts should be completely removed from broker and proxy and function
workers. There are ways to apply configs without doing search and replaces with
scripts and i think this is cleaner and less error prone.
For example, you can create a broker configmap with the configs you want to
override:
data:
broker.conf: |-
zookeeperServers=zk-0,zk-1,zk-2
configurationStoreServers=zk-0,zk-1,zk-2
clusterName=my-cluster
This config can be mounted as a file in the container and then you can set
this broker conf environment variable to where the file was mounted. For
example:
PULSAR_BROKER_CONF: /conf/broker.conf
Then you can just start the broker
```
./bin/pulsar broker
```
The broker will read the config you mounted through the configmap and merge
it with the default values.
If you want to add functions just add another config map with the function
config and add the following flag to where the file was mounted.
```
./bin/pulsar broker --functions-worker-conf /conf/functions_worker.yml
```
The same can be done for the proxy. I think this makes the deployments
easier and removes the search and replace scripts from the brokers and proxy.
The approach also allows you add any config in the future without having to
update a script.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
