Implement functionality to connect RSU Health Monitor to multiple RSUs(4) from same instance #606
Conversation
dan-du-car
changed the title
| * @param rsuConfigsStr A JSON string includes all RSUs related configrations. | ||
| * @return JSON::Value A JSON object that includes RSUS information. | ||
| */ | ||
| Json::Value parseJson(const std::string &rsuConfigsStr) const; |
There was a problem hiding this comment.
Why does parse JSON return a JSON value. Isn't the purpose of this object to store the configuration. I would imagine either we have a constructor or initializer that reads in the string and updates the object or we have a function that returns and instance of the object.
RSUConfigurationList parse_rsu_config(const std::string &json) {
...
}
...
RSUConfigurationList rsuConfig = parse_rsu_config(jsonString)Not saying this way is inherently wrong, just trying to understand why we return Json value and what your thought process was in setting it up this way.
There was a problem hiding this comment.
parseJson is a function used by parseRSUS() which is where we store the config objects. parseRSUS() does not return RSUConfigurationList because it stores the result to instance variable.
| @@ -125,7 +125,7 @@ namespace RSUHealthMonitor | |||
| try | |||
| { | |||
| // Create SNMP client and use SNMP V3 protocol | |||
| PLOG(logINFO) << "Update SNMP client: RSU IP: " << _rsuIp << ", RSU port: " << _snmpPort << ", User: " << _securityUser << ", auth pass phrase: " << _authPassPhrase << ", security level: " | |||
| PLOG(logINFO) << "SNMP client: RSU IP: " << _rsuIp << ", RSU port: " << _snmpPort << ", User: " << _securityUser << ", auth pass phrase: " << _authPassPhrase << ", security level: " | |||
There was a problem hiding this comment.
Not an issue for this PR but we should figure out how we want to handle external authentication information. Having it written to a log is probably bad practice. Having it stored anywhere as plain text is probably bad practice. Not sure what convention for this is.
There was a problem hiding this comment.
Isn't this authentication info provided from the UI?
There was a problem hiding this comment.
Yeah that is why I think this is probably not solvable right now but eventually we want to hide the authentication information you enter similar to a login.
| _rsuStatusTimer->ChangeFrequency(_timerThId, std::chrono::milliseconds(_interval * SEC_TO_MILLI)); | ||
| } | ||
| catch (const RSUConfigurationException &ex) | ||
| { | ||
| PLOG(logERROR) << "Cannot update RSU configurations due to error: " << ex.what(); |
There was a problem hiding this comment.
What do we want to happen when there is an error? Will this clear the previous list?
There was a problem hiding this comment.
It will use the previous list. No clear. In the parseRSUS() function, I left a comment saying that it will not clear the list until all configs are updated correctly.
There was a problem hiding this comment.
Is this what we want? Is it better to clear the list?
There was a problem hiding this comment.
Can you provide some scenarios where we need to clear this?
|
|
||
| namespace RSUHealthMonitor | ||
| { | ||
| class RSUConfigurationException : public tmx::TmxException |
There was a problem hiding this comment.
Does TmxException have a custom defined move constructor/operator that is not noexcept? I think this is the case since there is a code smell about this. Does TmxException provide any useful functionality here like a stack trace or something? Just curious
There was a problem hiding this comment.
It has stack trace function defined. However, it does not have move constructor.
| } | ||
| else | ||
| { | ||
| throw RSUConfigurationException("RSU IP does not exist."); |
There was a problem hiding this comment.
Shouldn't each RSU have an ID or name as well. Then instead of RSU IP does not exist we could have a more descriptive exception. IP is missing for RSU <name>. Otherwise when this exception is thrown and you have 4 RSU configs, only one has a miss spelling of the RSUIPKey then it maybe hard to find. Just a thought.
|
PR Details
Description
Update the V2X Hub's RSU Health Monitor plugin supports monitoring the health status of multiple Roadside Units (RSUs) per V2X Hub instance.
Related Issue
NA
Motivation and Context
NA
How Has This Been Tested?
Types of changes
Checklist:
V2XHUB Contributing Guide