Skip to content

HBASE-24195 : Admin.getRegionServers() should return live servers exc… #1523

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.

Sign up for GitHub

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

Merged
virajjasani merged 2 commits into from
Apr 16, 2020
Merged

HBASE-24195 : Admin.getRegionServers() should return live servers exc… #1523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea2cc71
HBASE-24195 : Admin.getRegionServers() should return live servers exc…
virajjasani Apr 15, 2020
d52a720
addressing review comments
virajjasani Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@

import java.io.Closeable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -64,6 +66,7 @@
import org.apache.hadoop.hbase.snapshot.UnknownSnapshotException;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.Pair;
import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils;
import org.apache.yetus.audience.InterfaceAudience;

/**
Expand Down Expand Up @@ -1064,7 +1067,31 @@ default Collection<ServerName> getBackupMasters() throws IOException {
* @throws IOException if a remote or network exception occurs
*/
default Collection<ServerName> getRegionServers() throws IOException {
return getClusterMetrics(EnumSet.of(Option.SERVERS_NAME)).getServersName();
return getRegionServers(false);
}

/**
* Retrieve all current live region servers including decommissioned
* if excludeDecommissionedRS is false, else non-decommissioned ones only
*
* @param excludeDecommissionedRS should we exclude decommissioned RS nodes
* @return all current live region servers including/excluding decommissioned hosts
* @throws IOException if a remote or network exception occurs
*/
default Collection<ServerName> getRegionServers(boolean excludeDecommissionedRS)
throws IOException {
List<ServerName> allServers =
getClusterMetrics(EnumSet.of(Option.SERVERS_NAME)).getServersName();
if (!excludeDecommissionedRS) {
return allServers;
}
List<ServerName> decommissionedRegionServers = listDecommissionedRegionServers();
if (CollectionUtils.isNotEmpty(decommissionedRegionServers)) {

@liuml07 liuml07 Apr 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

 if (CollectionUtils.isNotEmpty(decommissionedRegionServers)) {
    allServers = new ArrayList<>(allServers);
    allServers.removeIf(decommissionedRegionServers::contains);
    return Collections.unmodifiableList(allServers);
  }
 return allServers;

to

  return allServers.stream()
    .filter(s -> !decommissionedRegionServers.contains(s))
    .collect(ImmutableList.toImmutableList());

I think this will save one time full copy (previous code copy twice, and this copies just once). Need to confirm.

@virajjasani virajjasani Apr 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason behind copy (new ArrayList<>(allServers)) was making allServers unmodifiable list. So ImmutableList.toImmutableList() seems to be solving our problem.

allServers = new ArrayList<>(allServers);
allServers.removeIf(decommissionedRegionServers::contains);
return Collections.unmodifiableList(allServers);
}
return allServers;
}

/**
Expand Down
39 changes: 39 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -864,4 +865,42 @@ public void testSlowLogResponses() throws Exception {
Assert.assertEquals(onlineLogRecords.size(), 0);
}

@Test
public void testGetRegionServers() throws Exception {
// get all live server names
List<ServerName> serverNames = new ArrayList<>(ADMIN.getRegionServers(true));
Assert.assertEquals(3, serverNames.size());

List<ServerName> serversToDecom = new ArrayList<>();
ServerName serverToDecommission = serverNames.get(0);

serversToDecom.add(serverToDecommission);
ADMIN.decommissionRegionServers(serversToDecom, false);
waitForServerDecom(serverToDecommission, true);

Assert.assertEquals(2, ADMIN.getRegionServers(true).size());

@liuml07 liuml07 Apr 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Assert.assertEquals(3, ADMIN.getRegionServers(false).size()); for paranoid reviewers (cough cough me).

@virajjasani virajjasani Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, definitely

ADMIN.recommissionRegionServer(serverToDecommission, Collections.emptyList());

Assert.assertEquals(3, ADMIN.getRegionServers(true).size());
waitForServerDecom(serverToDecommission, false);

@liuml07 liuml07 Apr 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wait for decommission this server in the end?

@liuml07 liuml07 Apr 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the assertion Assert.assertEquals(3, ADMIN.getRegionServers(true).size()); should have made sure all RS are fine now. I understand in case that is not enough, you explicitly make sure the mini cluster is ready for other test cases. OK, good.

@virajjasani virajjasani Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wait for decommission this server in the end?

Actually, we are waiting for server to be recommissioned (maybe method name not correct, last arg determines). Let me change the order.

}

private static void waitForServerDecom(ServerName excludeServer,
boolean anyServerDecommissioned) {
TEST_UTIL.waitFor(3000, () -> {
Comment thread
HorizonNet marked this conversation as resolved.
try {
List<ServerName> decomServers = TEST_UTIL.getAdmin().listDecommissionedRegionServers();
if (anyServerDecommissioned) {
return decomServers.size() == 1
&& decomServers.get(0).equals(excludeServer);
} else {
return decomServers.size() == 0;
}
} catch (IOException e) {
throw new RuntimeException(e);
}
});
}

}