Hi guys, I'm a new hire in Salesforce and still learning everthing about open source world and bookkeeper system. Recently we encountered a bug and I'm working on the fix. Before the fix I want to understand the root cause so may need some help from you.
The bug is that when we run the "bin/bookkeeper shell" start-up script with some options (in our case it's ledgeridformat -xxx option) it always ignore what we put into the command line option and only using the value from config file or default value. But before it did work without problem. After investigation on git history and code I finally found the commit which made it stop working: *commit 381bddcd771d994e91732c54a7705451d419ea31* *Author: eolivelli <[email protected] <[email protected]>>* *Date: Sat Jul 30 22:53:28 2016 -0700* * BOOKKEEPER-933: ClientConfiguration always inherits System properties* * Author: eolivelli <[email protected] <[email protected]>>* * Reviewers: Sijie Guo <[email protected] <[email protected]>>* * Closes #50 from eolivelli/BOOKKEEPER-933 and squashes the following commits:* * 0bcae1e [eolivelli] BOOKKEEPER-933 ClientConfiguration always inherits System properties* * beb68fb [eolivelli] BOOKKEEPER-933 ClientConfiguration always inherits System properties* Because the option is passed in by "Java -D" system properties, and this commit disabled system property loading by default. So the fix is simple, just add "-Dorg.apache.bookkeeper.conf.readsystemproperties=true" into the "bin/bookkeeper" script for each command line. But I don't want just quick fix rather make sure it's the right way to do things. I think most of people in this community are senior engineers we all know for a distributed system development we need follow some guidelines and best pratice to make sure the code base is going to the right way in long term. Firstly I want to know is "Java -D" (rather than parsing the args by code) the standard way to pass command line options in java world? Secondly I want to understand what's the reason to make that commit and changing the default logic before? Since all the system properties are passed in by "Java -D" command line, if don't need some properies why not just remove them from the command line? Now use this case as an example, there are actually several different places controlling the option value: - config file - command line option - environment variables - Java system property - even worse, there is a special Java system property enable/disable partial of them (environment variable and command line option). That make things very confusing and hard to debug. In my opinion for service we should ONLY control the behavior by config file. For this case it's not a service but a command line tool (shell script). But as a command line tool I think its behavior should only defined by command line option. Maybe using config file to define default options is fine, but should not use environment variable which is hidden from the user. And we should not provide a option to enable/disable other options. Imagine when user type this command line: *java org.apache.bookkeeper.bookie.BookieShell -DledgerIdFormatterClass=XXX -DentryFormatterClass=XXX -Dorg.apache.bookkeeper.conf.readsystemproperties=false* Just because the last option the previous options just ignored, how confusing is that? Actually user will not type above but using our bookkeeper shell script as this way: *bookkeeper shell ledgeridformat -uuid listledgers -Dorg.apache.bookkeeper.conf.readsystemproperties=false* That's more confusing because user don't know what is really disabled. As a command line tool user should expect everything based on what typed. Command line options should always be handled directly. If in Java world using system property is standard way (rather than parsing the options in code) to pass command line options, then we should never disable system property. And if there is case user don't want load some system property, they just don't put that -DXX=xx in the command line (this is another reason we should not use environment variable as a hiden default system property). By this way the tool behavior is well defined by config file and everything appeared in command line. Since I'm still new on Java world and Linux open source systems (they just prefer environment variables for everything? Actually I prefer to isolate service from environment as much as possible), my thoughts may be not suitable for this case or we may have different coding standard, any comments just let me know. Thanks.
