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