-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-18288. Total requests and total requests per sec served by RPC servers #4431
Conversation
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
@@ -123,7 +123,7 @@ public void testRequeueCall() throws Exception { | |||
+ CommonConfigurationKeys.IPC_BACKOFF_ENABLE, true); | |||
|
|||
NameNodeAdapter.getRpcServer(nn).refreshCallQueue(configuration); | |||
assertEquals(1, NameNodeAdapter.getRpcServer(nn).getTotalRequests()); | |||
assertTrue(NameNodeAdapter.getRpcServer(nn).getTotalRequests() > 0); |
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.
if you have an assertion where a useful message isn't autogenerated the way assertequals does, afraid you will need to add a message or use assertJ to describe the assert. sorry.
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 agree, this is bit painful for debugging purpose. Updated with assertJ as it has better APIs to deal with such conditions (assertThat(x).isGreaterThan(y)
).
Updated with the same, thanks for the suggestion @steveloughran
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! it is pretty verbose for the little things, but i find assertj lovely for complex things, especially collections
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.
javadocs good, but now you've moved to assertTrue i have to esk for the failure message for the assert. think of whoever tries to debug a failing jenkins build: what information do they need.
assertj asserts are great here, FWIW
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.
On the whole, it looks good to me. Add some comments.
long totalRequestsDiff = currentTotalRequests - lastSeenTotalRequests; | ||
lastSeenTotalRequests = currentTotalRequests; | ||
if ((currentTime - lastExecuted) > 0) { | ||
double totalRequestsPerMillis = |
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 unit here is second
, so the variable name should be changed.
try { | ||
RpcMetrics rpcMetrics = server.getRpcMetrics(); | ||
assertEquals(0, rpcMetrics.getTotalRequests()); | ||
assertEquals(0.0, rpcMetrics.getTotalRequestsPerSecond(), 0.0); |
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.
RpcMetrics#getTotalRequestsPerSecond return long
, so we can make some change.
assertEquals(0.0, rpcMetrics.getTotalRequestsPerSecond(), 0.0); | |
assertEquals(0, rpcMetrics.getTotalRequestsPerSecond()); |
Thanks @tomscut, addressed comments. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
LGTM.
+1 from me, merge into trunk and branch-3.3 |
Thanks @virajjasani for your contribution! Thanks @steveloughran for your review! |
Hi @virajjasani , I found some conflicts when cherry-pick this to branch-3.3. For security, could you please submit a new PR to branch-3.3 separately? Thanks a lot! |
… servers (apache#4431) Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Tao Li <tomscut@apache.org>
Description of PR
RPC Servers provide bunch of useful information like num of open connections, slow requests, num of in-progress handlers, RPC processing time, queue time etc, however so far it doesn't provide accumulation of all requests as well as current snapshot of requests per second served by the server. Exposing them would benefit from operational viewpoint in identifying how busy the servers have been and how much load they are currently serving in the presence of cluster wide high load.
How was this patch tested?
Local dev cluster and UT.
For code changes: