-
Notifications
You must be signed in to change notification settings - Fork 531
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
YAML loading of mbuf_size and max_msgs #387
Conversation
@shailesh33 do not merge yet it is not ready. Created the PR to gather some feedback (see my other comments). |
@@ -326,6 +326,8 @@ struct server_pool { | |||
struct endpoint stats_endpoint; /* stats_listen: socket info for stats */ | |||
int stats_interval; /* stats aggregation interval */ | |||
bool enable_gossip; /* enable/disable gossip */ | |||
size_t mbuf_chunk_size; /* mbuf chunk size */ |
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.
@shailesh33 Previously we would use mbuf-size
in the command line arguments and in the code it would appear mbuf_chunk_size
. For max-msgs
, in the code, it would appear alloc_msgs_max
. Which version do we want to use in the YAML mbuf_size
to be simpler or mbuf_chunk_size
to be the same as in the code? Similarly for max_msgs
or alloc_msgs_max
?
@@ -5,4 +5,6 @@ dyn_o_mite: | |||
servers: | |||
- 127.0.0.1:22122:1 | |||
data_store: 0 | |||
mbuf_size: 16384 |
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.
@shailesh33 During the phase of supporting both of them, mbuf_size
and max_msgs
will be optional in YAML, but it would be nice to include them in the conf/dynomite.yml
for OSS folks to get used to setting them properly.
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.
makes sense. Thank you!
@@ -16,3 +16,5 @@ dyn_o_mite: | |||
data_store: 0 | |||
stats_listen: 0.0.0.0:22224 | |||
enable_gossip: false | |||
mbuf_size: 16384 |
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.
@shailesh33 adding in only one configuration file for test reasons...
ebbec81
to
a5de961
Compare
@shailesh33 the PR is ready for review! |
@@ -53,8 +53,8 @@ To build Dynomite in _debug mode_: | |||
-o, --output=S : set logging file (default: stderr) | |||
-c, --conf-file=S : set configuration file (default: conf/dynomite.yml) | |||
-p, --pid-file=S : set pid file (default: off) | |||
-m, --mbuf-size=N : set size of mbuf chunk in bytes (default: 16384 bytes) | |||
-M, --max-msgs=N : set max number of messages to allocate (default: 200000) | |||
-m, --mbuf-size=N : (deprecated - use YAML) set size of mbuf chunk in bytes (default: 16384 bytes) |
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.
use Configuration file?
Since we are talking about configuration file in the earlier arguments?
@shailesh33 updated the |
case 'm': | ||
value = dn_atoi(optarg, strlen(optarg)); | ||
case 'm': // deprecated argument | ||
loga("-m or --mbuf-size command line arguments has been deprecated. Use YAML"); |
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.
Use configuration file
@@ -564,11 +558,15 @@ init_test(int argc, char **argv) | |||
|
|||
test_pre_run(&nci); | |||
|
|||
core_start(&nci); |
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.
wondering if this is correct? Did you run it?
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.
src/dynomite-test fails.
@@ -319,15 +319,15 @@ mbuf_split(struct mhdr *h, uint8_t *pos, func_mbuf_copy_t cb, void *cbarg) | |||
|
|||
/** | |||
* Initialize memory buffers to store network packets/socket buffers. | |||
* @param[in,out] nci Dynomite instance. | |||
* @param[in,out] mbuf_chunk_size |
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.
change the argument name in the comments
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.
Looks good. Thank you for your effort
Final arguments moving to YAML.