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
HBASE-24913 Refactor TestJMXConnectorServer #2286
Conversation
🎊 +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.
Patch looks good except for one item.
String cps = JMXListener.class.getName() + "," + MyAccessController.class.getName(); | ||
conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, cps); | ||
conf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, cps); | ||
rmiRegistryPort = UTIL.randomFreePort(); |
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.
While the port might have been 'free' when we got it, by the time we go to use it, it may have been occupied for another. See HBaseTestingUtility#setupMiniKdc where it loops until no BindException. This trick is used in a few places to get around port clash.
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 @saintstack for reviewing.
For JMXConnectorServer starting, only use the default port or specifying port, can't find a free port by retrying. So I use HBaseTestingUtility#randomFreePort to get a free port, which mark ports as taken and don't return repeated ports. This patch may not completely avoid port clash, but at least it can reduce the possibility ?
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.
In my experience it is strange how often we meet clashes. Any chance of catching BindException and retrying?
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 port used to start JMXConnectorServer is obtained from the configuration during cluster startup. And just log even if failed to start JMXConnectorServer because of port clash.
So if we want to avoid port clash absolutely, restart cluster?
Oh, want to say why you are making these changes? You having port clashes running tests? What is the difference in test time after these changes? |
I just saw the code and found that maybe it can be optimized here. It saves 3 mins on my PC and 1 min in Test Results. |
Thanks. Good. |
admin.close(); | ||
UTIL.shutdownMiniCluster(); | ||
} | ||
|
||
@Before | ||
public void setUp() { | ||
hasAccess = false; |
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.
Seems only the first call will throw AccessDeny exception. Did you know why this design?
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.
HMaster#stopMaster doesn't catch any exception that cpHost.preStopMaster()
throws out.
HRegionServer#stop catches exception from rsHost.preStop
.
But I have no idea why the two are different in the design of handling exceptions.
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
Signed-off-by: Guanghao Zhang <zghao@apache.org>
Signed-off-by: Guanghao Zhang <zghao@apache.org>
Signed-off-by: Guanghao Zhang <zghao@apache.org>
Signed-off-by: Guanghao Zhang <zghao@apache.org>
Signed-off-by: Guanghao Zhang <zghao@apache.org> (cherry picked from commit 39ebc3e) Change-Id: If28501f3ae890d47b077f54e46d1a63f583f83ae
Two optimization points for TestJMXConnectorServer in this issue: