You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Zhimeng Shi <zh...@gmail.com> on 2017/01/24 20:36:00 UTC
About command line option handling
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 <eolivelli@gmail.com <eo...@gmail.com>>*
*Date: Sat Jul 30 22:53:28 2016 -0700*
* BOOKKEEPER-933: ClientConfiguration always inherits System properties*
* Author: eolivelli <eolivelli@gmail.com <eo...@gmail.com>>*
* Reviewers: Sijie Guo <sijie@apache.org <si...@apache.org>>*
* 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.
Re: About command line option handling
Posted by Flavio P JUNQUEIRA <fp...@apache.org>.
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" <zh...@gmail.com> 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 <eolivelli@gmail.com <eo...@gmail.com>>*
> *Date: Sat Jul 30 22:53:28 2016 -0700*
>
> * BOOKKEEPER-933: ClientConfiguration always inherits System properties*
>
> * Author: eolivelli <eolivelli@gmail.com <eo...@gmail.com>>*
>
> * Reviewers: Sijie Guo <sijie@apache.org <si...@apache.org>>*
>
> * 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.
>
Re: About command line option handling
Posted by Sijie Guo <si...@apache.org>.
On Tue, Jan 24, 2017 at 12:36 PM, Zhimeng Shi <zh...@gmail.com> 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 <eolivelli@gmail.com <eo...@gmail.com>>*
> *Date: Sat Jul 30 22:53:28 2016 -0700*
>
> * BOOKKEEPER-933: ClientConfiguration always inherits System properties*
>
> * Author: eolivelli <eolivelli@gmail.com <eo...@gmail.com>>*
>
> * Reviewers: Sijie Guo <sijie@apache.org <si...@apache.org>>*
>
> * 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.
>