-
Notifications
You must be signed in to change notification settings - Fork 24
Centralize configuration parameters in a Key/Value store #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
0fa9d2c to
440a522
Compare
jaharkes
left a comment
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.
Although this is the direction I think I would like to go with config, I am currently not convinced this is the right change at the moment.
| } | ||
| } | ||
|
|
||
| void VenusConf::load_default_config() |
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.
A little odd to have the venus default config in coda-src/util/
Would we end up with the codasrv, updateclnt, updatesrv, auth2 configs here as well?
Or, if venusconf is venus specific, it probably shouldn't be in /util/
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.
Thanks. That's actually wrong. It should be in coda-src/util/. I put it there first for convenience and forgot to move it into coda-src/venus/.
| static int sftp_windowsize = UNSET_WS; | ||
| static int sftp_sendahead = UNSET_SA; | ||
| static int sftp_ackpoint = UNSET_AP; | ||
| static int sftp_packetsize = UNSET_PS; |
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 are some variables static globals resolved at init time, while others are processed in-line? Why is there a distinction between f.i. sftp_windowsize and rpc2_retries?
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 reason is simple. If the variable is used all over the .cc, I decided to declare a static global. If it's only needed once I tent to process it in-line. Do you believe that we should declare all static? I was also thinking to do something like;
struct comm_config {
int COPModes;
int sftp_windowsize;
int sftp_sendahead;
int sftp_ackpoint;
int sftp_packetsize;
int rpc2_retries;
...
}
static struct comm_config config = {0, UNSET_WS, UNSET_SA, UNSET_AP ....};
...
void CommInit() {
config.COPModes = GetVenusConf().get_int_value("copmodes");
config.sftp_windowsize = GetVenusConf().get_int_value("sftp_windowsize");
config.sftp_sendahead = GetVenusConf().get_int_value("sftp_sendahead");
config.sftp_ackpoint = GetVenusConf().get_int_value("sftp_ackpoint");
config.sftp_packetsize = GetVenusConf().get_int_value("sftp_packetsize");
config.rpc2_retries = GetVenusConf().get_int_value("RPC2_retries");
...
}This for every sub-system would be consistent and expressive.
| BEGIN_HTML | ||
| <a name="ViceUpdateDB"><strong>Ensure the incore copy of the databases is up to date. | ||
| </strong></a> | ||
| </strong></a> |
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.
only spaces in comments updated in this file
lib-src/base/codaconfparser.cc
Outdated
| } | ||
| #endif | ||
|
|
||
| #include "codaconfparser.h" |
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 bother with a .cc when everything is in the header file.
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.
I normally like to leave the placeholder for extensions, and try to have a .cc and .h per class. But I'll remove it.
lib-src/base/Makefile.am
Outdated
| dllist.h dllist.c getpeereid.h getpeereid.c \ | ||
| parse_realms.h parse_realms.c urlquote.h urlquote.c codaenv.c codaenv.h | ||
| codaconf.c codaconf.h stringkeyvaluestore.cc stringkeyvaluestore.h \ | ||
| coda_flock.c codaconfparser.cc codaconfparser.h codaconffileparser.cc \ |
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.
Are these (codaconfparser, codaconffileparser, codaconfcmdlineparser)'s actually used anywhere?
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.
Yes, CodaConfFileParser and CodaConfCmdLineParser inherit from CodaConfParser and are used in venus.cc.
|
Even if this isn't the right change at the moment, I'd like to push this a little forward to a point where it's consistent and merge-ready. I'd just like mention that with this we invert the break the dependency loop between main ( This improves testability and decoupling substantially. For instance, I was able to test CacheFile completely independent from everything else. |
440a522 to
e170fff
Compare
* Change int type to int64_t * Set the correct default values to some configuration parameters * Move re-entering barriers to parent class * Finish implementing apply_consistency_rules and check
e170fff to
4e7cdab
Compare
4e7cdab to
0ca15ff
Compare
No description provided.