-
Notifications
You must be signed in to change notification settings - Fork 58
refactor(util): use flags to define configurations #275
Conversation
This comment has been minimized.
This comment has been minimized.
DSN_DEFINE_bool("tools.simple_logger", fast_flush, false, "whether to flush immediately"); | ||
|
||
DSN_DEFINE_bool("tools.simple_logger", | ||
short_header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the value of one config format isn't right, for example, size = 1MB
and size=1024
, what happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is exactly the same as dsn_config_get_value_int64
like formerly it does.
src/core/core/flags.cpp
Outdated
class flag_registry : public utils::singleton<flag_registry> | ||
{ | ||
public: | ||
flag_registry() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to move it to private
for singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
singleton
requires constructor to be public, or the compilation wil fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make ::utils::singleton<flag_registry> to be friend class of flag_registry may help.
After this patch, we can provide a way to modify configs at runtime (just in-memory configs, not configs in file) |
}; | ||
|
||
#define FLAG_REG_CONSTRUCTOR(type, type_enum) \ | ||
flag_registerer::flag_registerer( \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the name is flag_registerer
, but not flag_register
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These class names are learned from gflags. If users are familiar with gflags, he will soon get started to use dsn flags.
See this: https://github.com/gflags/gflags/blob/master/src/gflags.h.in#L432.
"registerer" plays the role to register the flag, where "register" is a verb, not a noun.
Introduction
flags.h defines a series of utilities that provide gflags-style config definition. The main difference is that gflags uses command-line flags, our implementation reads "config.ini" for flags.
Let's see an example of the new flag API:
for each of the replicas,
the current implementation has to specify the config section, description, default value, and read the config value, though all replicas use the same config value. After this PR, the implementation can be simplified to:
Compared to the original configuration API, which reads configuration item every time it is used (though config file is pre-loaded, reading the item still costs one ex-lock operation), flags.h inits all configurations at the start time, freeing our developer in some way.
From the framework's perspective, all the flags are inited at
dsn_run()
, after the configurations are loaded. Of course, you cannot use the flag beforedsn::flags_initialize()
.Side effects
This PR doesn't change any behavior related to configurations. It only introduces a new utility.
Manual Tests
1. Tests flag validation works
Use config
stderr_start_level == LOG_LEVEL_INVALID
, the validation failed and generated a coredump.Outputs:
2. Tests correct flag value is loaded
Set config
short_header = true
. Output:Set config
short_header = false
. Apparently,short_header
is well loaded. The logger prints function's name ahead of the message: