-
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-18060.RPCMetrics increases the number of handlers in processing. #3822
Conversation
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.
Left few comments, thanks for this nice idea @jianghuazhu
@@ -498,6 +498,7 @@ protected ResponseBuffer initialValue() { | |||
private Map<Integer, Listener> auxiliaryListenerMap; | |||
private Responder responder = null; | |||
private Handler[] handlers = null; | |||
final private AtomicInteger numInProcessHandler = new AtomicInteger(); |
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.
nit: private final
private enum HandlerStatus { | ||
/**Working for a Call.*/ | ||
IN_PROCESS, | ||
|
||
/**Idle state, waiting for a new call.*/ | ||
IN_IDLE | ||
} |
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.
Do we need this enum? We are updating the status to either IN_PROCESS
or IN_IDLE
but not really exposing it anywhere.
assertEquals(rpcMetrics.getNumInProcessHandler(), 0); | ||
|
||
ExternalCall<String> call1 = newExtCall(ugi, | ||
new PrivilegedExceptionAction<String>() { |
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.
nit: use lambda?
} | ||
}); | ||
ExternalCall<Void> call2 = newExtCall(ugi, | ||
new PrivilegedExceptionAction<Void>() { |
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.
same as above, lambda and assertEquals for exact comparison
new PrivilegedExceptionAction<String>() { | ||
@Override | ||
public String run() throws Exception { | ||
assertTrue(rpcMetrics.getNumInProcessHandler() > 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.
How about exact comparison? We can use assertEquals(1, rpcMetrics.getNumInProcessHandler())
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.
Thank you @virajjasani for your comments and reviews.
I will update it later.
💔 -1 overall
This message was automatically generated. |
93c168d
to
9f4fb72
Compare
💔 -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.
+1 (non-binding)
Thank you very much @virajjasani . |
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.
Had a cursory look:
In case of Observer reads, if the namenode state is lower than that of the client. We would reQueue the call, and reattempt. Will this numInProcessHandler
not keep on increasing in that case?
Can you double check, I couldn't dig in further...
// Re-queue the call and continue
requeueCall(call);
call = null;
continue;
Thanks @ayushtkn for the comment and review. |
Thank you @jianghuazhu for your patch. Would you update Metrics.md as well to reflect the new metric to the document? |
Thank you @aajisaka for your comments and reviews. |
9f4fb72
to
703e6f3
Compare
🎊 +1 overall
This message was automatically generated. |
I have updated some, can you please review this pr again, @aajisaka . |
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. This should be created as a HADOOP JIRA.
I updated it again. Move the theme to the Hadoop-common project. |
Thanks @jianghuazhu for your update. LGTM. |
@jianghuazhu Thanks for your contribution. @aajisaka @ayushtkn @tomscut Thanks for your reviews! Merged |
Description of PR
Now we can't see how many Handlers in RPC are actually being used. It would be very helpful to see this information directly through RPCMetrics.
The purpose of this pr is to solve this problem.
Details: HDFS-16394
How was this patch tested?
This needs to be tested.
When accessing RPC, you need to know how many handlers are being used based on RPCMetrics.