Duplicate flags when running bin/pulsar bookie

[cckellogg]

Describe the bug
A recent change (#5675) was made to the the bin/pulsar script. This change loads the bkenv.sh file and then sets the opts when running the bookie. This produces duplicate flags when running a bookie.

/usr/bin/java -cp /Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/conf:::/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/lib/: -Dlog4j.configurationFile=log4j2.yaml -Djute.maxbuffer=10485760 -Djava.net.preferIPv4Stack=true -Dpulsar.allocator.exit_on_oom=true -Dio.netty.recycler.maxCapacity.default=1000 -Dio.netty.recycler.linkCapacity=1024 -Xms2g -Xmx2g -XX:MaxDirectMemorySize=4g -XX:+UseG1GC -XX:MaxGCPauseMillis=10 -XX:+ParallelRefProcEnabled -XX:+UnlockExperimentalVMOptions -XX:+AggressiveOpts -XX:+DoEscapeAnalysis -XX:ParallelGCThreads=32 -XX:ConcGCThreads=32 -XX:G1NewSizePercent=50 -XX:+DisableExplicitGC -XX:-ResizePLAB -Dpulsar.log.appender=RoutingAppender -Dpulsar.log.dir=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/logs -Dpulsar.log.level=info -Dpulsar.routing.appender.default=Console -Dpulsar.functions.process.container.log.dir=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/logs -Dpulsar.functions.java.instance.jar=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/instances/java-instance.jar -Dpulsar.functions.python.instance.file=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/instances/python-instance/python_instance_main.py -Dpulsar.functions.extra.dependencies.dir=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/instances/deps -Dpulsar.functions.instance.classpath=/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/conf:::/Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/lib/: -Xms2g -Xmx2g -XX:MaxDirectMemorySize=2g -XX:+UseG1GC -XX:MaxGCPauseMillis=10 -XX:+ParallelRefProcEnabled -XX:+UnlockExperimentalVMOptions -XX:+AggressiveOpts -XX:+DoEscapeAnalysis -XX:ParallelGCThreads=32 -XX:ConcGCThreads=32 -XX:G1NewSizePercent=50 -XX:+DisableExplicitGC -XX:-ResizePLAB -Dio.netty.leakDetectionLevel=disabled -Dio.netty.recycler.maxCapacity.default=1000 -Dio.netty.recycler.linkCapacity=1024 -Dpulsar.log.file=bookkeeper.log org.apache.bookkeeper.proto.BookieServer --conf /Users/ckellogg/apps/apache-pulsar-2.6.0-SNAPSHOT/conf/bookkeeper.conf

For example the Xms and XX:MaxDirectMemory are set twice. What is the behavior if flags are set twice? What value gets used?

To Reproduce
Steps to reproduce the behavior:

1. bin/pulsar bookie

[murong00]

@sijie @jiazhai The change in #5675 is not consistent with our previous discussion in #4555, the original intention of bin/pulsar is to use pulsar_env.sh and the only requirement is for people to add extra bookie opts.

[jiazhai]

@murong00 Thanks for the notice. Thanks @cckellogg for the report.
If the java run time options provided twice, in the most of JVM version, winner will be the right-most value. In above case bookie config -Xms2g -Xmx2g -XX:MaxDirectMemorySize=2g should take effect.
It is a little confuse, But seems no easy way to solve this issue.

@murong00 From my point of view, 5675 is not too much conflict with 4555, in current code, we could have all the env in bkenv exported, and in these shell scripts, they could be used directly. so that avoid the need to set extra bookie opts in different places.

[cckellogg]

@jiazhai the main issue I see with #5675 is it's a breaking change. Users who update to a newer version of pulsar will have to change their configurations and that's not a good user experience. Also, all of the kubernetes charts default to using PULSAR_MEM to set resources for the bookies and that will no longer work.

I agree with @murong00 only variables from pulsar_env.sh should be loaded by bin/pulsar

[jiazhai]

@cckellogg Thanks for the feedback.
Using PULSAR_MEM instead of BOOKIE_MEM to set bookies, this seems be not a good way.
Is it OK to change the k8s charts to use BOOKIE_MEM for bookies setting?

[cckellogg]

@jiazhai my main concern is the current behavior was to use PULSAR_MEM to set the memory for bookies when running bin/pulsar now that is not the case so all users that upgrade to a newer version will have to change their configuration before upgrading. Also, this is provides duplicate flags which is confusing regardless if the right most one takes.

In my opinion bkenv.sh should only be applied to bin/bookkeeper and the pulsar_env.sh should only be used in the bin/pulsar and other bin/pulsar-* scripts. To me loading both is confusing and can lead to unexpected errors or behavior.

Why is PULSAR_MEM not a good way to set the memory?
PULSAR_MEM is used to set the memory for all commands (zookeeper, presto, function-worker, etc..) in the bin/pulsar script. It seems clear to me if I run bin/pulsar or any bin/pulsar-* script I control the memory with PULSAR_MEM. #5675 changes this and makes it inconsistent with how everything else works and that in my opinion is confusing.

[jiazhai]

@cckellogg Thanks for bring up this discussion.
4555 and 5675 both will bring in duplicate of flags when execute bin/pulsar bookie.
4555 resolved the issue that pulsar scripts not support BOOKIE settings.
5675 loaded BOOKIE_EXTRA_OPTS, but not using it unless it is really needed(like in command bin/pulsar bookie), it will not effect other non-bookie commands.

From my view, It is still the issue that raised in my above comments:

Using PULSAR_MEM instead of BOOKIE_MEM to set bookies

Is this a good way or not?

From 4555 and 4675, seems it puzzled some user by use PULSAR_MEM instead of BOOKIE_MEM to run bin/pulsar bookie.
I am not persist on these change too much. Only want to make it more smooth for user to understanding, and avoid changing backwards and forwards in the future.

also would like to bring @murong00 @hangc0276 into the discussion.

[murong00]

@jiazhai The motivation of 4555 is to pass the BOOKIE_EXTRA_OPTS (just for passing jaas file when enable kerberos) when using pulsar-daemon, as sijie's comment in 4555 it doesn't use BOOKIE_MEM and BOOKIE_GC.

I agree with @cckellogg to some extent PULSAR_MEM is used to set the memory for all command，from my point of view bookeeper is also a part of pulsar.

[hangc0276]

I agree with @jiazhai ,the key problem of the issue is:

Using PULSAR_MEM instead of BOOKIE_MEM to set bookies
this way is good or not?
when start bookie, the right configuration will be used, the BOOKIE_MEM will override PULSAR_MEM

In my opinion, PULSAR_MEM should represent pulsar memory configuration and BOOKIE_MEM should represent bookie memory configuration. For a newcomer to pulsar, he will be confused to set bkenv.sh to configure bookie memory, but not work.

If you just using PULSAR_MEM to start bookie, that will be a little confused.

For old pulsar version upgrading, I suggest to add a note in upgrade guide.

It is not elegant to override PULSAR_MEM using BOOKIE_MEM when starting bookie, the best way of starting bookie only using BOOKIE_MEM.

[hangc0276]

@jiazhai The motivation of 4555 is to pass the BOOKIE_EXTRA_OPTS (just for passing jaas file when enable kerberos) when using pulsar-daemon, as sijie's comment in 4555 it doesn't use BOOKIE_MEM and BOOKIE_GC.

I agree with @cckellogg to some extent PULSAR_MEM is used to set the memory for all command，from my point of view bookeeper is also a part of pulsar.

When only start bookie using PULSAR_MEM, it will force user to copy pulsar directory to another one the start bookie, otherwise you can't set bookie's memory independently.

[murong00]

@hangc0276 yes, it is not elegant to override the setting, from my point view I can accept both as long as the doc is detailed. The issue here is duplicate flags may confuse some users.

[sijie]

@murong00 @jiazhai @hangc0276 @cckellogg any conclusion on this issue?

[cckellogg]

I think we need to resolve this issue there are still duplicate flags when starting a bookie by running pulsar/bookie. This is confusing to users and causing unexpected issues. I think this change (#5675) should be reverted before 2.6.

https://stackoverflow.com/questions/2740725/duplicated-java-runtime-options-what-is-the-order-of-preference