On Tue, Jan 24, 2017 at 12:36 PM, Zhimeng Shi <[email protected]> wrote:
> 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. > Yes. the intention for this fix is to disable system properties by default. in general, it is a bit too hard to track where a setting is from when the settings can be from system properties, files and other sources. So in this change, we disabled it by default. I believed this change has been merged since Jul 30th last year. I am a bit surprised that you encountered recently. Also, I am not sure if this change is yet released. Enabling "-Dorg.apache.bookkeeper.conf.readsystemproperties=true" in bin/bookkeeper script is the right solution for this, as the reason I stated above. > > 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? > -D is not a way to pass command line options. command line args are a bit different from client settings. when developing a client, there is no way for a client to take command line options as the client can be run as part of tool, or another services. so for any client settings, it will be read from ClientConfiguration. You can load the client settings from files, setting the client setttings specifically in your program when using client. > 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? > Removing the default behavior for loading settings from system properties mainly for managebility. Image you have a program that running two bookkeeper clients. Those two clients might have two different settings, if allowing system properties, it would potentially screw up the settings reflected in those two clients. > > 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). > I don't think we load settings from command line option. You have to distinguish client settings and command line options. These are differentt > > 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. > Again, I think first let's try to differentiate what are the command line options and what are the client settings. The points you are raising here trying to mix them together. for command line options, they are the options used by the command line tool itself. for the client settings, they are the settings used by the client (the client can be used in the command line tool, other services and such). So the main concern you have is how shall we load client settings in a command line tool that use a client? In this case, I would suggest fixing the bookkeeper shell to only use configuration file, which it is the most clear solution. tool related options are from command line, client related settings are from configuration files. Let me know if it makes sense to you. > > 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. >
