It is better to create a jira issue and report your findings in the issue. Thanks! -Flavio
On 24 Jan 2017 20:36, "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. > > 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. >
