wolfstudy 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_r353541860
 
 

 ##########
 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.
   
   ok, I will fix them.
   
   > 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.
   
   Yes, this change is only used to fix problems encountered by users during 
the current use.
   
   >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.
   
   The proposal looks good to me,  this is a big change, maybe we can open a 
PIP to discuss further details.

----------------------------------------------------------------
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

Reply via email to