caroliney14 commented on a change in pull request #2769:
URL: https://github.com/apache/hbase/pull/2769#discussion_r600032198
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
##########
@@ -176,7 +176,7 @@ private void testSanity(final String testName) throws
Exception {
@Test
public void testRegionAssignmentAfterMasterRecoveryDueToZKExpiry() throws
Exception {
MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
- cluster.startRegionServer();
+ cluster.startRegionServerAndWait(2000);
Review comment:
> and both time out if the rs cannot be found within that list within
the given timeout.
@saintstack What I meant by this is, the behavior of the
`HBaseCluster.waitForRegionServerToStart` and
`MiniHBaseCluster.startRegionServerAndWait` methods are such that the methods
themselves will time out and throw an exception if the rs is not found in
master's online servers list within a specified time period. The method
shouldn't time out if the master/rs communication logic is correct (when I
tested locally, it doesn't). See method pasted below:
```
/**
* Wait for the specified region server to join the cluster
* @throws IOException if something goes wrong or timeout occurs
*/
public void waitForRegionServerToStart(String hostname, int port, long
timeout)
throws IOException {
long start = System.currentTimeMillis();
while ((System.currentTimeMillis() - start) < timeout) {
for (ServerName server :
getClusterMetrics().getLiveServerMetrics().keySet()) {
if (server.getHostname().equals(hostname) && server.getPort() ==
port) {
return;
}
}
Threads.sleep(100);
}
throw new IOException("did timeout " + timeout + "ms waiting for region
server to start: "
+ hostname);
}
```
As for changing where rs's `online` flag is set -- I guess it depends on
what we wish to define as "online" for a regionserver. If "online" for a
regionserver means that the regionserver has finished setup, then it's fine
where it is. If "online" for a regionserver should mean that the regionserver
has communicated to master it's ready to be assigned regions, then `online`
should be set after the first `reportForDuty`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
