Add load_factor to server_info in reporting mode #3817
Conversation
* load_factor was missing from server_info when the server was running in reporting mode. Now, the reporting mode server calls server_info on the p2p node, and propagates the load_factor back to the client.
|
@ximinez Do you think we should set |
Yeah, I think that would be smart, and better than an error in this case. |
| @@ -38,6 +40,17 @@ doServerInfo(RPC::JsonContext& context) | |||
| context.params.isMember(jss::counters) && | |||
| context.params[jss::counters].asBool()); | |||
|
|
|||
| if (context.app.config().reporting()) | |||
There was a problem hiding this comment.
I think this can be simplified into a try ... catch such as:
try:
ret[field] = proxied[result][jssinfo][load_factor]
catch:
ret[field] = 1
There was a problem hiding this comment.
This wouldn't work with a try/catch because operator[] doesn't throw. However, the const version returns a nullValue type, which we can test for.
Json::Value const proxied = forwardToP2p(context);
auto const lf = proxied[jss::result][jss::info][jss::load_factor];
ret[jss::info][jss::load_factor] = lf.isNull() ? 1: lf;
| @@ -38,6 +40,17 @@ doServerInfo(RPC::JsonContext& context) | |||
| context.params.isMember(jss::counters) && | |||
| context.params[jss::counters].asBool()); | |||
|
|
|||
| if (context.app.config().reporting()) | |||
There was a problem hiding this comment.
This wouldn't work with a try/catch because operator[] doesn't throw. However, the const version returns a nullValue type, which we can test for.
Json::Value const proxied = forwardToP2p(context);
auto const lf = proxied[jss::result][jss::info][jss::load_factor];
ret[jss::info][jss::load_factor] = lf.isNull() ? 1: lf;
| Json::Value proxied = forwardToP2p(context); | ||
| if (proxied.isMember(jss::result) && | ||
| proxied[jss::result].isMember(jss::info) && | ||
| proxied[jss::result][jss::info].isMember(jss::load_factor)) | ||
| ret[jss::info][jss::load_factor] = | ||
| proxied[jss::result][jss::info][jss::load_factor]; | ||
| else | ||
| ret[jss::info][jss::load_factor] = 1; |
There was a problem hiding this comment.
Is this testable? Can you add or update a unit test for it?
There was a problem hiding this comment.
No this is not unit testable. Proxying requires running two servers and should be tested in integration/QA.
There was a problem hiding this comment.
Not really testable without adding some type of virtual proxy. This functionality (reporting) will be split into its own code base, so I think this stuff will be removed from rippled itself in that project.
|
@ximinez What changes are you requesting on this? I see your suggestion about the try-catch and the unit test comment. I don't think this change is unit testable, and I interpreted the try-catch comment as just a suggestion. Let me know what you actually want me to change here. |
I try to mark significant suggestions as "changes requested" so I know I need to follow-up after a change is made, or there's a discussion to not do it. |
Got it, thanks for clarifying |
cjcobb23
added
the
Passed
|
I don't use reporting mode, but I assume this is already in |
load_factor was missing from server_info when the server was running in reporting mode. This commit fixes that by querying the p2p node for the load_factor, and propagating that info back to the client. If the p2p node is unavailable or returns an error, load_factor is set to 1.
This change is