Add 'reconnect' to prdcr_start* to deprecate 'interval' #1240
Conversation
| @@ -334,10 +339,15 @@ attr=<value> | |||
| .br | |||
| A regular expression | |||
| .TP | |||
| .BI [interval " interval"] | |||
| .BI [reconnect " interval"] | |||
There was a problem hiding this comment.
We should support strings like '20s'. Specifying the reconnect interval in microseconds is pretty silly and error prone. Since we're renaming the attribute, this is the time to do it.
There was a problem hiding this comment.
I'll submit the change to support strings in another patch.
There was a problem hiding this comment.
Please put it in this patch. We are logically introducing a 'new' attribute or at least an alias with expanded syntax; this would be the place to get the documentation and the implementation all sync'd up. This patch is already pretty concise.
ldms/src/ldmsd/ldmsd_request.c
Outdated
| if (!interval_str) { | ||
| interval_str = ldmsd_req_attr_str_value_get_by_id(reqc, | ||
| LDMSD_ATTR_INTERVAL); | ||
| ovis_log(config_log, OVIS_LWARN, "The 'interval' in " |
There was a problem hiding this comment.
I don't think we need these messages in the server.
There was a problem hiding this comment.
The log message in the server is for the case that the 'interval' attribute is used in configuration files.
ldms/src/ldmsd/ldmsd_request.c
Outdated
| @@ -1729,11 +1729,17 @@ static int prdcr_start_handler(ldmsd_req_ctxt_t reqc) | |||
|
|
|||
| ldmsd_req_ctxt_sec_get(reqc, &sctxt); | |||
| interval_str = ldmsd_req_attr_str_value_get_by_id(reqc, | |||
| LDMSD_ATTR_INTERVAL); | |||
| LDMSD_ATTR_RECONNECT); | |||
There was a problem hiding this comment.
Why do we need a new attribute?
There was a problem hiding this comment.
The only reason I added LDMSD_ATTR_RECONNECT is to differentiate between the uses of 'interval' and 'reconnect' in configuration files, so LDMSD could give a warning message that the 'interval' attribute is being deprecated.
There was a problem hiding this comment.
Hi @nichamon, I understand, but my suggestion is that the server shouldn't do this, ldmsd_controller, ldmsdctl and the config parsing should.
There was a problem hiding this comment.
I pushed a patch that gets rid of the new attribute LDMSD_ATTR_RECONNECT and let the parsers for ldmsd_controller, ldmsctl, and config files give the warning about 'interval' being deprecated.
nichamon
force-pushed
the
prdcr_start-reconnect
branch
from
46df69c
to
70a04ac
Compare
nichamon
force-pushed
the
prdcr_start-reconnect
branch
2 times, most recently
from
4bbcfad
to
b5caa4f
Compare
|
@nichamon, is this pull request obsolete? |
nichamon
force-pushed
the
prdcr_start-reconnect
branch
from
b5caa4f
to
31e88a6
Compare
No, it is not. I just rebased it on top of OVIS-4, and it is ready to merge. |
No description provided.