From f81347b1c772482ae5a45d7709c6aae9d2c32b57 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Wed, 7 Feb 2024 13:20:35 +0100 Subject: [PATCH 01/16] WIP: modify admin API and protos to encode new boolean --- .../java/org/apache/hadoop/hbase/client/Admin.java | 12 ++++++++++++ .../hadoop/hbase/client/AdminOverAsyncAdmin.java | 6 ++++++ .../org/apache/hadoop/hbase/client/AsyncAdmin.java | 3 +++ .../apache/hadoop/hbase/client/AsyncHBaseAdmin.java | 6 ++++++ .../hadoop/hbase/client/RawAsyncHBaseAdmin.java | 11 +++++++++++ .../hbase/shaded/protobuf/RequestConverter.java | 6 ++++++ .../src/main/protobuf/server/master/Master.proto | 1 + .../hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java | 5 +++++ .../hadoop/hbase/thrift2/client/ThriftAdmin.java | 4 ++++ 9 files changed, 54 insertions(+) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index c0e2994c3c5e..67480cb5d4ca 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -2265,6 +2265,18 @@ default boolean replicationPeerModificationSwitch(boolean on) throws IOException */ void decommissionRegionServers(List servers, boolean offload) throws IOException; + /** + * Mark region server(s) as decommissioned to prevent additional regions from getting assigned to + * them. Optionally unload the regions on the servers. If there are multiple servers to be + * decommissioned, decommissioning them at the same time can prevent wasteful region movements. + * Region unloading is asynchronous. + * @param servers The list of servers to decommission. + * @param offload True to offload the regions from the decommissioned servers + * @param matchHostNameOnly True to prevent the hostname from ever joining again, regardless of startTime + * @throws IOException if a remote or network exception occurs + */ + void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) throws IOException; + /** * List region servers marked as decommissioned, which can not be assigned regions. * @return List of decommissioned region servers. diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java index c13dfc33e3d2..54c4cb5797d2 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java @@ -929,6 +929,12 @@ public void decommissionRegionServers(List servers, boolean offload) get(admin.decommissionRegionServers(servers, offload)); } + @Override + public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) + throws IOException { + get(admin.decommissionRegionServers(servers, offload, matchHostNameOnly)); + } + @Override public List listDecommissionedRegionServers() throws IOException { return get(admin.listDecommissionedRegionServers()); diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java index bdb0228d9687..242d2d7a6950 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java @@ -1146,6 +1146,9 @@ CompletableFuture isProcedureFinished(String signature, String instance */ CompletableFuture decommissionRegionServers(List servers, boolean offload); + CompletableFuture decommissionRegionServers(List servers, boolean offload, + boolean matchHostNameOnly); + /** * List region servers marked as decommissioned, which can not be assigned regions. * @return List of decommissioned region servers wrapped by {@link CompletableFuture} diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java index 69f353600036..73825132d04a 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java @@ -635,6 +635,12 @@ public CompletableFuture decommissionRegionServers(List server return wrap(rawAdmin.decommissionRegionServers(servers, offload)); } + @Override + public CompletableFuture decommissionRegionServers(List servers, + boolean offload, boolean matchHostNameOnly) { + return wrap(rawAdmin.decommissionRegionServers(servers, offload, matchHostNameOnly)); + } + @Override public CompletableFuture> listDecommissionedRegionServers() { return wrap(rawAdmin.listDecommissionedRegionServers()); diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index 103a64e520a1..91d257b2dd37 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -2517,6 +2517,17 @@ DecommissionRegionServersResponse, Void> call(controller, stub, .call(); } + @Override + public CompletableFuture decommissionRegionServers(List servers, + boolean offload, + boolean matchHostNameOnly) { + return this. newMasterCaller() + .action((controller, stub) -> this. call(controller, stub, + RequestConverter.buildDecommissionRegionServersRequest(servers, offload, matchHostNameOnly), + (s, c, req, done) -> s.decommissionRegionServers(c, req, done), resp -> null)) + .call(); + } @Override public CompletableFuture> listDecommissionedRegionServers() { return this.> newMasterCaller() diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java index ce12aaea0d24..3c1cb97125d4 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java @@ -1562,6 +1562,12 @@ public static GetQuotaStatesRequest buildGetQuotaStatesRequest() { .addAllServerName(toProtoServerNames(servers)).setOffload(offload).build(); } + public static DecommissionRegionServersRequest + buildDecommissionRegionServersRequest(List servers, boolean offload, boolean matchHostNameOnly) { + return DecommissionRegionServersRequest.newBuilder() + .addAllServerName(toProtoServerNames(servers)).setOffload(offload).setMatchHostNameOnly(matchHostNameOnly).build(); + } + public static RecommissionRegionServerRequest buildRecommissionRegionServerRequest(ServerName server, List encodedRegionNames) { RecommissionRegionServerRequest.Builder builder = RecommissionRegionServerRequest.newBuilder(); diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto index a8adaa27453f..cf5d4efa618c 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto @@ -696,6 +696,7 @@ message ListDecommissionedRegionServersResponse { message DecommissionRegionServersRequest { repeated ServerName server_name = 1; required bool offload = 2; + optional bool matchHostNameOnly = 3; } message DecommissionRegionServersResponse { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java index 35c868413e19..2948aefeeff2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java @@ -715,6 +715,11 @@ public void decommissionRegionServers(List servers, boolean offload) admin.decommissionRegionServers(servers, offload); } + public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) + throws IOException { + admin.decommissionRegionServers(servers, offload, matchHostNameOnly); + } + public List listDecommissionedRegionServers() throws IOException { return admin.listDecommissionedRegionServers(); } diff --git a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java index 0eff84bba7c8..a88563dd0e87 100644 --- a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java +++ b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java @@ -1069,7 +1069,11 @@ public boolean isReplicationPeerEnabled(String peerId) throws IOException { @Override public void decommissionRegionServers(List servers, boolean offload) { throw new NotImplementedException("decommissionRegionServers not supported in ThriftAdmin"); + } + @Override + public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) { + throw new NotImplementedException("decommissionRegionServers not supported in ThriftAdmin"); } @Override From fda82eb3562f0d33474d761bcddbd80864d96fb0 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Wed, 7 Feb 2024 15:04:59 +0100 Subject: [PATCH 02/16] Formatted RawAsyncHBaseAdmin class. --- .../org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index 91d257b2dd37..47e5cc4cb8b0 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -2519,8 +2519,7 @@ DecommissionRegionServersResponse, Void> call(controller, stub, @Override public CompletableFuture decommissionRegionServers(List servers, - boolean offload, - boolean matchHostNameOnly) { + boolean offload, boolean matchHostNameOnly) { return this. newMasterCaller() .action((controller, stub) -> this. call(controller, stub, From a1b18969a212d970e1dcdc40d33ed0bb94e74111 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Wed, 7 Feb 2024 15:20:30 +0100 Subject: [PATCH 03/16] Locate the API in HMaster and started refactoring two paths for decommissioning the regionserver --- .../main/java/org/apache/hadoop/hbase/master/HMaster.java | 6 ++++++ .../org/apache/hadoop/hbase/master/MasterRpcServices.java | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 88b82f01069e..72111e3742af 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -4016,6 +4016,12 @@ public boolean isReplicationPeerModificationEnabled() { * @param servers Region servers to decommission. */ public void decommissionRegionServers(final List servers, final boolean offload) + throws IOException { + this.decommissionRegionServers(servers, offload, false); + } + + public void decommissionRegionServers(final List servers, final boolean offload, + final boolean matchHostNameOnly) throws IOException { List serversAdded = new ArrayList<>(servers.size()); // Place the decommission marker first. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index 1da8e03d179e..1116b3262220 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -2279,10 +2279,11 @@ public DecommissionRegionServersResponse decommissionRegionServers(RpcController List servers = request.getServerNameList().stream() .map(pbServer -> ProtobufUtil.toServerName(pbServer)).collect(Collectors.toList()); boolean offload = request.getOffload(); + boolean matchHostNameOnly = request.getMatchHostNameOnly(); if (server.cpHost != null) { server.cpHost.preDecommissionRegionServers(servers, offload); } - server.decommissionRegionServers(servers, offload); + server.decommissionRegionServers(servers, offload, matchHostNameOnly); if (server.cpHost != null) { server.cpHost.postDecommissionRegionServers(servers, offload); } From 71329ffff3b630d7d58ab585e326b839e52906aa Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Wed, 7 Feb 2024 15:23:05 +0100 Subject: [PATCH 04/16] Renamed matchHostNameOnly to snakecase --- .../src/main/protobuf/server/master/Master.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto index cf5d4efa618c..73a29aec809b 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto @@ -696,7 +696,7 @@ message ListDecommissionedRegionServersResponse { message DecommissionRegionServersRequest { repeated ServerName server_name = 1; required bool offload = 2; - optional bool matchHostNameOnly = 3; + optional bool match_host_name_only = 3; } message DecommissionRegionServersResponse { From 68c662fdb8b4cfa65145fd02aa3178d7927878af Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Wed, 7 Feb 2024 15:24:32 +0100 Subject: [PATCH 05/16] Added default value for the optional match_host_name_only in the Master.proto file --- .../src/main/protobuf/server/master/Master.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto index 73a29aec809b..bd49b2bba5e2 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto @@ -696,7 +696,7 @@ message ListDecommissionedRegionServersResponse { message DecommissionRegionServersRequest { repeated ServerName server_name = 1; required bool offload = 2; - optional bool match_host_name_only = 3; + optional bool match_host_name_only = 3 [default = false]; } message DecommissionRegionServersResponse { From 707f824bbd732156efc712f0701db94025d669e6 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 9 Feb 2024 15:46:06 +0100 Subject: [PATCH 06/16] Ran spotless on previously touched files and continued refactoring of the code. --- .../org/apache/hadoop/hbase/client/Admin.java | 10 ++++--- .../hbase/client/AdminOverAsyncAdmin.java | 4 +-- .../hbase/client/RawAsyncHBaseAdmin.java | 6 ++-- .../shaded/protobuf/RequestConverter.java | 7 +++-- .../hbase/master/DrainingServerTracker.java | 29 +++++++++++++------ .../apache/hadoop/hbase/master/HMaster.java | 3 +- .../hadoop/hbase/master/ServerManager.java | 15 +++++----- .../hbase/rsgroup/VerifyingRSGroupAdmin.java | 4 +-- .../hbase/thrift2/client/ThriftAdmin.java | 3 +- 9 files changed, 48 insertions(+), 33 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index 67480cb5d4ca..af014093ae7c 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -2270,12 +2270,14 @@ default boolean replicationPeerModificationSwitch(boolean on) throws IOException * them. Optionally unload the regions on the servers. If there are multiple servers to be * decommissioned, decommissioning them at the same time can prevent wasteful region movements. * Region unloading is asynchronous. - * @param servers The list of servers to decommission. - * @param offload True to offload the regions from the decommissioned servers - * @param matchHostNameOnly True to prevent the hostname from ever joining again, regardless of startTime + * @param servers The list of servers to decommission. + * @param offload True to offload the regions from the decommissioned servers + * @param matchHostNameOnly True to prevent the hostname from ever joining again, regardless of + * startTime * @throws IOException if a remote or network exception occurs */ - void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) throws IOException; + void decommissionRegionServers(List servers, boolean offload, + boolean matchHostNameOnly) throws IOException; /** * List region servers marked as decommissioned, which can not be assigned regions. diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java index 54c4cb5797d2..8e14d7a2b272 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java @@ -930,8 +930,8 @@ public void decommissionRegionServers(List servers, boolean offload) } @Override - public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) - throws IOException { + public void decommissionRegionServers(List servers, boolean offload, + boolean matchHostNameOnly) throws IOException { get(admin.decommissionRegionServers(servers, offload, matchHostNameOnly)); } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index 47e5cc4cb8b0..b483b5458273 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -2523,10 +2523,12 @@ public CompletableFuture decommissionRegionServers(List server return this. newMasterCaller() .action((controller, stub) -> this. call(controller, stub, - RequestConverter.buildDecommissionRegionServersRequest(servers, offload, matchHostNameOnly), - (s, c, req, done) -> s.decommissionRegionServers(c, req, done), resp -> null)) + RequestConverter.buildDecommissionRegionServersRequest(servers, offload, + matchHostNameOnly), + (s, c, req, done) -> s.decommissionRegionServers(c, req, done), resp -> null)) .call(); } + @Override public CompletableFuture> listDecommissionedRegionServers() { return this.> newMasterCaller() diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java index 3c1cb97125d4..96f3ef7129c1 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java @@ -1562,10 +1562,11 @@ public static GetQuotaStatesRequest buildGetQuotaStatesRequest() { .addAllServerName(toProtoServerNames(servers)).setOffload(offload).build(); } - public static DecommissionRegionServersRequest - buildDecommissionRegionServersRequest(List servers, boolean offload, boolean matchHostNameOnly) { + public static DecommissionRegionServersRequest buildDecommissionRegionServersRequest( + List servers, boolean offload, boolean matchHostNameOnly) { return DecommissionRegionServersRequest.newBuilder() - .addAllServerName(toProtoServerNames(servers)).setOffload(offload).setMatchHostNameOnly(matchHostNameOnly).build(); + .addAllServerName(toProtoServerNames(servers)).setOffload(offload) + .setMatchHostNameOnly(matchHostNameOnly).build(); } public static RecommissionRegionServerRequest diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java index 41f5709e911a..c626e2987f34 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java @@ -19,8 +19,8 @@ import java.io.IOException; import java.util.List; -import java.util.NavigableSet; -import java.util.TreeSet; +import java.util.NavigableMap; +import java.util.TreeMap; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.zookeeper.ZKListener; @@ -51,7 +51,7 @@ public class DrainingServerTracker extends ZKListener { private static final Logger LOG = LoggerFactory.getLogger(DrainingServerTracker.class); private ServerManager serverManager; - private final NavigableSet drainingServers = new TreeSet<>(); + private final NavigableMap drainingServers = new TreeMap<>(); private Abortable abortable; public DrainingServerTracker(ZKWatcher watcher, Abortable abortable, @@ -72,14 +72,21 @@ public void start() throws KeeperException, IOException { serverManager.registerListener(new ServerListener() { @Override public void serverAdded(ServerName sn) { - if (drainingServers.contains(sn)) { + if (isServerInDrainedList(sn)) { serverManager.addServerToDrainList(sn); } } }); List servers = ZKUtil.listChildrenAndWatchThem(watcher, watcher.getZNodePaths().drainingZNode); - add(servers); + if (servers != null) { + add(servers); + } + } + + private boolean isServerInDrainedList(ServerName sn) { + return drainingServers.containsKey(sn.getHostname()) + || drainingServers.containsKey(sn.getServerName()); } private void add(final List servers) throws IOException { @@ -87,17 +94,21 @@ private void add(final List servers) throws IOException { this.drainingServers.clear(); for (String n : servers) { final ServerName sn = ServerName.valueOf(ZKUtil.getNodeName(n)); - this.drainingServers.add(sn); + this.drainingServers.put(n, sn); this.serverManager.addServerToDrainList(sn); - LOG.info("Draining RS node created, adding to list [" + sn + "]"); - + LOG.info("Draining RS node created, adding to list [{}]", sn); } } } private void remove(final ServerName sn) { synchronized (this.drainingServers) { - this.drainingServers.remove(sn); + if (drainingServers.containsKey(sn.getHostname())) { + this.drainingServers.remove(sn.getHostname()); + } else { + this.drainingServers.remove(sn.getServerName()); + } + // TODO: refactor the removeServerFromDrainList method below this.serverManager.removeServerFromDrainList(sn); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 72111e3742af..3701a90bf2e1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -4021,8 +4021,7 @@ public void decommissionRegionServers(final List servers, final bool } public void decommissionRegionServers(final List servers, final boolean offload, - final boolean matchHostNameOnly) - throws IOException { + final boolean matchHostNameOnly) throws IOException { List serversAdded = new ArrayList<>(servers.size()); // Place the decommission marker first. String parentZnode = getZooKeeper().getZNodePaths().drainingZNode; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java index 2afd48c58df5..64e0a054a315 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java @@ -649,10 +649,9 @@ public synchronized void moveFromOnlineToDeadServers(final ServerName sn) { public synchronized boolean removeServerFromDrainList(final ServerName sn) { // Warn if the server (sn) is not online. ServerName is of the form: // , , - if (!this.isServerOnline(sn)) { - LOG.warn("Server " + sn + " is not currently online. " - + "Removing from draining list anyway, as requested."); + LOG.warn( + "Server {} is not currently online. Removing from draining list anyway, as requested.", sn); } // Remove the server from the draining servers lists. return this.drainingServers.remove(sn); @@ -667,18 +666,18 @@ public synchronized boolean addServerToDrainList(final ServerName sn) { // , , if (!this.isServerOnline(sn)) { - LOG.warn("Server " + sn + " is not currently online. " - + "Ignoring request to add it to draining list."); + LOG.warn("Server {} is not currently online. Ignoring request to add it to draining list.", + sn); return false; } // Add the server to the draining servers lists, if it's not already in // it. if (this.drainingServers.contains(sn)) { - LOG.warn("Server " + sn + " is already in the draining server list." - + "Ignoring request to add it again."); + LOG.warn("Server {} is already in the draining server list.Ignoring request to add it again.", + sn); return true; } - LOG.info("Server " + sn + " added to draining server list."); + LOG.info("Server {} added to draining server list.", sn); return this.drainingServers.add(sn); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java index 2948aefeeff2..671331b5f5c7 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java @@ -715,8 +715,8 @@ public void decommissionRegionServers(List servers, boolean offload) admin.decommissionRegionServers(servers, offload); } - public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) - throws IOException { + public void decommissionRegionServers(List servers, boolean offload, + boolean matchHostNameOnly) throws IOException { admin.decommissionRegionServers(servers, offload, matchHostNameOnly); } diff --git a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java index a88563dd0e87..c7052f532f3b 100644 --- a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java +++ b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java @@ -1072,7 +1072,8 @@ public void decommissionRegionServers(List servers, boolean offload) } @Override - public void decommissionRegionServers(List servers, boolean offload, boolean matchHostNameOnly) { + public void decommissionRegionServers(List servers, boolean offload, + boolean matchHostNameOnly) { throw new NotImplementedException("decommissionRegionServers not supported in ThriftAdmin"); } From 9ca8d61848c7668396cce6f9c19979a2d0e7c5bc Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 9 Feb 2024 16:13:07 +0100 Subject: [PATCH 07/16] Modified ZKUtil and HMaster to pass in the matchHostNameOnly boolean flag. --- .../protobuf/server/zookeeper/ZooKeeper.proto | 4 +++ .../apache/hadoop/hbase/master/HMaster.java | 2 +- .../apache/hadoop/hbase/zookeeper/ZKUtil.java | 26 ++++++++++++++++++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/zookeeper/ZooKeeper.proto b/hbase-protocol-shaded/src/main/protobuf/server/zookeeper/ZooKeeper.proto index 17fa31ffbe69..5d166a1d4846 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/zookeeper/ZooKeeper.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/zookeeper/ZooKeeper.proto @@ -107,3 +107,7 @@ message DeprecatedTableState { message SwitchState { optional bool enabled = 1; } + +message DrainedZNodeServerData { + optional bool match_host_name_only = 1 [default = false]; +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 3701a90bf2e1..cc4df851ab0c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -4028,7 +4028,7 @@ public void decommissionRegionServers(final List servers, final bool for (ServerName server : servers) { try { String node = ZNodePaths.joinZNode(parentZnode, server.getServerName()); - ZKUtil.createAndFailSilent(getZooKeeper(), node); + ZKUtil.createAndFailSilent(getZooKeeper(), node, matchHostNameOnly); } catch (KeeperException ke) { throw new HBaseIOException( this.zooKeeper.prefix("Unable to decommission '" + server.getServerName() + "'."), ke); diff --git a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java index be8f25cc39b0..121ae0f48bdb 100644 --- a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java +++ b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java @@ -51,6 +51,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; /** * Internal HBase utility class for ZooKeeper. @@ -746,12 +747,35 @@ public static void asyncCreate(ZKWatcher zkw, String znode, byte[] data, /** * Creates the specified node, iff the node does not exist. Does not set a watch and fails * silently if the node already exists. The node created is persistent and open access. + * The node will contain a byte array of data that denotes that the host shouldn't be + * drained all the time and that the full servername (with port and startcode) should + * be matched when recommissioning it. * @param zkw zk reference * @param znode path of node * @throws KeeperException if unexpected zookeeper exception */ public static void createAndFailSilent(ZKWatcher zkw, String znode) throws KeeperException { - createAndFailSilent(zkw, znode, new byte[0]); + createAndFailSilent(zkw, znode, false); + } + + /** + * Creates the specified node, iff the node does not exist. Does not set a watch and fails + * silently if the node already exists. The node created is persistent and open access. + * The node will contain a byte array of data that denotes whether host should or shouldn't be + * considered drained, if it's `true` then the host will be rejected from rejoining the cluster + * regardless of port and startcode; otherwise, any other full servername with a different port, + * startcode or both will be able to join and this unique servername will still be considered + * drained. + * @param zkw zk reference + * @param znode path of node + * @param matchHostNameOnly whether hostname should be always considered drained or not + * @throws KeeperException if unexpected zookeeper exception + */ + public static void createAndFailSilent(ZKWatcher zkw, String znode, boolean matchHostNameOnly) + throws KeeperException { + byte[] data = DrainedZNodeServerData.newBuilder().setMatchHostNameOnly(matchHostNameOnly) + .build().toByteArray(); + createAndFailSilent(zkw, znode, data); } /** From 7d8d1c59a30a761f9b600a39371df42efd00d91b Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 9 Feb 2024 16:53:17 +0100 Subject: [PATCH 08/16] Moved the zknode creation with data out of ZKUtil --- .../apache/hadoop/hbase/master/HMaster.java | 8 +++++- .../apache/hadoop/hbase/zookeeper/ZKUtil.java | 26 +------------------ 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index cc4df851ab0c..026df57bd3da 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -293,6 +293,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.RequestConverter; import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse; import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos.SnapshotDescription; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; /** * HMaster is the "master server" for HBase. An HBase cluster has one active master. If many masters @@ -4028,7 +4029,12 @@ public void decommissionRegionServers(final List servers, final bool for (ServerName server : servers) { try { String node = ZNodePaths.joinZNode(parentZnode, server.getServerName()); - ZKUtil.createAndFailSilent(getZooKeeper(), node, matchHostNameOnly); + // Encode whether the host should be decommissioned regardless of port + startcode or not + // in the znode's data + byte[] data = DrainedZNodeServerData.newBuilder().setMatchHostNameOnly(matchHostNameOnly) + .build().toByteArray(); + // Create a node with binary data + ZKUtil.createAndFailSilent(getZooKeeper(), node, data); } catch (KeeperException ke) { throw new HBaseIOException( this.zooKeeper.prefix("Unable to decommission '" + server.getServerName() + "'."), ke); diff --git a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java index 121ae0f48bdb..be8f25cc39b0 100644 --- a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java +++ b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java @@ -51,7 +51,6 @@ import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.shaded.protobuf.generated.ReplicationProtos; -import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; /** * Internal HBase utility class for ZooKeeper. @@ -747,35 +746,12 @@ public static void asyncCreate(ZKWatcher zkw, String znode, byte[] data, /** * Creates the specified node, iff the node does not exist. Does not set a watch and fails * silently if the node already exists. The node created is persistent and open access. - * The node will contain a byte array of data that denotes that the host shouldn't be - * drained all the time and that the full servername (with port and startcode) should - * be matched when recommissioning it. * @param zkw zk reference * @param znode path of node * @throws KeeperException if unexpected zookeeper exception */ public static void createAndFailSilent(ZKWatcher zkw, String znode) throws KeeperException { - createAndFailSilent(zkw, znode, false); - } - - /** - * Creates the specified node, iff the node does not exist. Does not set a watch and fails - * silently if the node already exists. The node created is persistent and open access. - * The node will contain a byte array of data that denotes whether host should or shouldn't be - * considered drained, if it's `true` then the host will be rejected from rejoining the cluster - * regardless of port and startcode; otherwise, any other full servername with a different port, - * startcode or both will be able to join and this unique servername will still be considered - * drained. - * @param zkw zk reference - * @param znode path of node - * @param matchHostNameOnly whether hostname should be always considered drained or not - * @throws KeeperException if unexpected zookeeper exception - */ - public static void createAndFailSilent(ZKWatcher zkw, String znode, boolean matchHostNameOnly) - throws KeeperException { - byte[] data = DrainedZNodeServerData.newBuilder().setMatchHostNameOnly(matchHostNameOnly) - .build().toByteArray(); - createAndFailSilent(zkw, znode, data); + createAndFailSilent(zkw, znode, new byte[0]); } /** From 49fd19bb8f3535b67144e849c03cef0cd72681ee Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 14:43:17 +0100 Subject: [PATCH 09/16] Refactored DrainingServerTracker to consult ZK before removing servers from the list. --- .../hbase/master/DrainingServerTracker.java | 63 ++++++++++++++----- .../apache/hadoop/hbase/master/HMaster.java | 39 ++++++++++-- .../hadoop/hbase/master/ServerManager.java | 8 +-- 3 files changed, 84 insertions(+), 26 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java index c626e2987f34..501b75eac12f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java @@ -19,18 +19,23 @@ import java.io.IOException; import java.util.List; -import java.util.NavigableMap; -import java.util.TreeMap; +import java.util.NavigableSet; +import java.util.TreeSet; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.zookeeper.ZKListener; import org.apache.hadoop.hbase.zookeeper.ZKUtil; import org.apache.hadoop.hbase.zookeeper.ZKWatcher; +import org.apache.hadoop.hbase.zookeeper.ZNodePaths; import org.apache.yetus.audience.InterfaceAudience; import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.com.google.protobuf.InvalidProtocolBufferException; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; + /** * Tracks the list of draining region servers via ZK. *

@@ -51,7 +56,7 @@ public class DrainingServerTracker extends ZKListener { private static final Logger LOG = LoggerFactory.getLogger(DrainingServerTracker.class); private ServerManager serverManager; - private final NavigableMap drainingServers = new TreeMap<>(); + private final NavigableSet drainingServers = new TreeSet<>(); private Abortable abortable; public DrainingServerTracker(ZKWatcher watcher, Abortable abortable, @@ -77,24 +82,28 @@ public void serverAdded(ServerName sn) { } } }); + List servers = ZKUtil.listChildrenAndWatchThem(watcher, watcher.getZNodePaths().drainingZNode); + if (servers != null) { add(servers); } } private boolean isServerInDrainedList(ServerName sn) { - return drainingServers.containsKey(sn.getHostname()) - || drainingServers.containsKey(sn.getServerName()); + return drainingServers.contains(sn); } private void add(final List servers) throws IOException { synchronized (this.drainingServers) { - this.drainingServers.clear(); + // Clear all servers from the draining list that shouldn't stay drained. Information about + // whether a server should stay drained or not is added to the ZNode by the HMaster + this.drainingServers.removeIf(sn -> !shouldServerStayDrained(sn)); + for (String n : servers) { final ServerName sn = ServerName.valueOf(ZKUtil.getNodeName(n)); - this.drainingServers.put(n, sn); + this.drainingServers.add(sn); this.serverManager.addServerToDrainList(sn); LOG.info("Draining RS node created, adding to list [{}]", sn); } @@ -103,21 +112,40 @@ private void add(final List servers) throws IOException { private void remove(final ServerName sn) { synchronized (this.drainingServers) { - if (drainingServers.containsKey(sn.getHostname())) { - this.drainingServers.remove(sn.getHostname()); - } else { - this.drainingServers.remove(sn.getServerName()); + if (shouldServerStayDrained(sn)) { + LOG.info( + "Refusing to remove drained RS {} from the list, it's marked as permanently drained", sn); + return; } - // TODO: refactor the removeServerFromDrainList method below + this.drainingServers.remove(sn); this.serverManager.removeServerFromDrainList(sn); + LOG.info("Successfully removed drained RS {} from the list", sn); } } + private boolean shouldServerStayDrained(final ServerName sn) { + boolean shouldBePermanentlyDecommissioned = false; + String parentZnode = this.watcher.getZNodePaths().drainingZNode; + String node = ZNodePaths.joinZNode(parentZnode, sn.getServerName()); + + try { + byte[] rawData = ZKUtil.getData(this.watcher, node); + // Check if the data is present for backwards compatibility, some nodes may not have it + if (rawData != null && rawData.length > 0) { + DrainedZNodeServerData znodeData = DrainedZNodeServerData.parseFrom(rawData); + shouldBePermanentlyDecommissioned = znodeData.getMatchHostNameOnly(); + } + } catch (InterruptedException | KeeperException | InvalidProtocolBufferException e) { + // pass + } + return shouldBePermanentlyDecommissioned; + } + @Override public void nodeDeleted(final String path) { if (path.startsWith(watcher.getZNodePaths().drainingZNode)) { final ServerName sn = ServerName.valueOf(ZKUtil.getNodeName(path)); - LOG.info("Draining RS node deleted, removing from list [" + sn + "]"); + LOG.info("Draining RS node deleted, removing from list [{}]", sn); remove(sn); } } @@ -128,10 +156,11 @@ public void nodeChildrenChanged(final String path) { try { final List newNodes = ZKUtil.listChildrenAndWatchThem(watcher, watcher.getZNodePaths().drainingZNode); - add(newNodes); - } catch (KeeperException e) { - abortable.abort("Unexpected zk exception getting RS nodes", e); - } catch (IOException e) { + + if (newNodes != null) { + add(newNodes); + } + } catch (KeeperException | IOException e) { abortable.abort("Unexpected zk exception getting RS nodes", e); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 026df57bd3da..eef4ad71a3cc 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -282,6 +282,7 @@ import org.apache.hbase.thirdparty.com.google.common.io.Closeables; import org.apache.hbase.thirdparty.com.google.gson.JsonParseException; import org.apache.hbase.thirdparty.com.google.protobuf.Descriptors; +import org.apache.hbase.thirdparty.com.google.protobuf.InvalidProtocolBufferException; import org.apache.hbase.thirdparty.com.google.protobuf.Service; import org.apache.hbase.thirdparty.org.eclipse.jetty.server.Server; import org.apache.hbase.thirdparty.org.eclipse.jetty.server.ServerConnector; @@ -4029,8 +4030,12 @@ public void decommissionRegionServers(final List servers, final bool for (ServerName server : servers) { try { String node = ZNodePaths.joinZNode(parentZnode, server.getServerName()); - // Encode whether the host should be decommissioned regardless of port + startcode or not - // in the znode's data + // Encode whether the host should be decommissioned permanently regardless of its + // port + startCode combination or not in the znode's data + if (matchHostNameOnly) { + LOG.info("Marking the host of '{}' as permanently decommissioned in ZooKeeper", + server.getServerName()); + } byte[] data = DrainedZNodeServerData.newBuilder().setMatchHostNameOnly(matchHostNameOnly) .build().toByteArray(); // Create a node with binary data @@ -4079,8 +4084,31 @@ public void recommissionRegionServer(final ServerName server, // Remove the server from decommissioned (draining) server list. String parentZnode = getZooKeeper().getZNodePaths().drainingZNode; String node = ZNodePaths.joinZNode(parentZnode, server.getServerName()); + boolean shouldBePermanentlyDecommissioned = false; + + // Get the binary data in the node and check whether the hostname is marked as + // permanently decommissioned or not + try { + byte[] rawData = ZKUtil.getData(getZooKeeper(), node); + + // Check if the data is present for backwards compatibility, some nodes may not have it + if (rawData != null && rawData.length > 0) { + DrainedZNodeServerData znodeData = DrainedZNodeServerData.parseFrom(rawData); + shouldBePermanentlyDecommissioned = znodeData.getMatchHostNameOnly(); + } + } catch (InterruptedException | KeeperException | InvalidProtocolBufferException e) { + throw new HBaseIOException(this.zooKeeper.prefix("Unable to recommission " + + server.getServerName() + ", was unable to read the node's data"), e); + } + try { - ZKUtil.deleteNodeFailSilent(getZooKeeper(), node); + if (shouldBePermanentlyDecommissioned) { + LOG.info("Skipping recommissioning of server {} because it was marked as permanently" + + " decommissioned in ZooKeeper", server.getServerName()); + return; + } else { + ZKUtil.deleteNodeFailSilent(getZooKeeper(), node); + } } catch (KeeperException ke) { throw new HBaseIOException( this.zooKeeper.prefix("Unable to recommission '" + server.getServerName() + "'."), ke); @@ -4103,8 +4131,9 @@ public void recommissionRegionServer(final ServerName server, } RegionInfo hri = regionState.getRegion(); if (server.equals(regionState.getServerName())) { - LOG.info("Skipping move of region " + hri.getRegionNameAsString() - + " because region already assigned to the same server " + server + "."); + LOG.info( + "Skipping move of region {} because region already assigned to the same server {}.", + hri.getRegionNameAsString(), server); continue; } RegionPlan rp = new RegionPlan(hri, regionState.getServerName(), server); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java index 64e0a054a315..e0c66130293b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java @@ -94,10 +94,10 @@ *

* If a sever is known not to be running any more, it is called dead. The dead server needs to be * handled by a ServerShutdownHandler. If the handler is not enabled yet, the server can't be - * handled right away so it is queued up. After the handler is enabled, the server will be submitted - * to a handler to handle. However, the handler may be just partially enabled. If so, the server - * cannot be fully processed, and be queued up for further processing. A server is fully processed - * only after the handler is fully enabled and has completed the handling. + * handled right away, so it is queued up. After the handler is enabled, the server will be + * submitted to a handler to handle. However, the handler may be just partially enabled. If so, the + * server cannot be fully processed, and be queued up for further processing. A server is fully + * processed only after the handler is fully enabled and has completed the handling. */ @InterfaceAudience.Private public class ServerManager { From 91134bbcebead0bad76c1ceba00555dd9976308f Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 14:49:14 +0100 Subject: [PATCH 10/16] Formatting in ZKUtil --- .../src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java index be8f25cc39b0..cdd046a2e3f1 100644 --- a/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java +++ b/hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java @@ -874,6 +874,7 @@ private static void deleteNodeFailSilent(ZKWatcher zkw, DeleteNodeFailSilent dnf try { zkw.getRecoverableZooKeeper().delete(delete.getPath(), delete.getVersion()); } catch (KeeperException.NoNodeException nne) { + // no-op } catch (InterruptedException ie) { zkw.interruptedException(ie); } From 6d8b37e402e92b96fa192911ce63822ebde3ba83 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 14:54:49 +0100 Subject: [PATCH 11/16] Revert unnecessary change in DrainedServerTracker --- .../apache/hadoop/hbase/master/DrainingServerTracker.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java index 501b75eac12f..5d29f0b18cbb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java @@ -77,7 +77,7 @@ public void start() throws KeeperException, IOException { serverManager.registerListener(new ServerListener() { @Override public void serverAdded(ServerName sn) { - if (isServerInDrainedList(sn)) { + if (drainingServers.contains(sn)) { serverManager.addServerToDrainList(sn); } } @@ -90,11 +90,7 @@ public void serverAdded(ServerName sn) { add(servers); } } - - private boolean isServerInDrainedList(ServerName sn) { - return drainingServers.contains(sn); - } - + private void add(final List servers) throws IOException { synchronized (this.drainingServers) { // Clear all servers from the draining list that shouldn't stay drained. Information about From 36d70bedb2b7d883c568792592b9aa701097f6c2 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 16:22:47 +0100 Subject: [PATCH 12/16] Writing tests for the admin api --- .../hbase/client/RawAsyncHBaseAdmin.java | 11 ++- .../shaded/protobuf/RequestConverter.java | 3 +- .../hbase/master/DrainingServerTracker.java | 2 +- .../client/TestAsyncDecommissionAdminApi.java | 83 ++++++++++++++++--- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index b483b5458273..8de848e3ad39 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -2509,12 +2509,11 @@ public CompletableFuture getLocks() { @Override public CompletableFuture decommissionRegionServers(List servers, boolean offload) { - return this. newMasterCaller() - .action((controller, stub) -> this. call(controller, stub, - RequestConverter.buildDecommissionRegionServersRequest(servers, offload), - (s, c, req, done) -> s.decommissionRegionServers(c, req, done), resp -> null)) - .call(); + // By default, when we decommission a RegionServer we don't mark the hostname as permanently + // decommissioned and instead mark the server location (host + port + startCode) as such + boolean matchHostNameOnly = false; + + return this.decommissionRegionServers(servers, offload, matchHostNameOnly); } @Override diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java index 96f3ef7129c1..6151bb815120 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java @@ -1558,8 +1558,7 @@ public static GetQuotaStatesRequest buildGetQuotaStatesRequest() { public static DecommissionRegionServersRequest buildDecommissionRegionServersRequest(List servers, boolean offload) { - return DecommissionRegionServersRequest.newBuilder() - .addAllServerName(toProtoServerNames(servers)).setOffload(offload).build(); + return RequestConverter.buildDecommissionRegionServersRequest(servers, offload, false); } public static DecommissionRegionServersRequest buildDecommissionRegionServersRequest( diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java index 5d29f0b18cbb..6d2be3e220cb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DrainingServerTracker.java @@ -90,7 +90,7 @@ public void serverAdded(ServerName sn) { add(servers); } } - + private void add(final List servers) throws IOException { synchronized (this.drainingServers) { // Clear all servers from the draining list that shouldn't stay drained. Information about diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java index 659aa0d05c68..dfdb92ac1227 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java @@ -58,14 +58,14 @@ public void testAsyncDecommissionRegionServers() throws Exception { assertEquals(TEST_UTIL.getHBaseCluster().getLiveRegionServerThreads().size(), clusterRegionServers.size()); - HashMap> serversToDecommssion = new HashMap<>(); + HashMap> serversToDecommission = new HashMap<>(); // Get a server that has regions. We will decommission one of the servers, // leaving one online. int i; for (i = 0; i < clusterRegionServers.size(); i++) { List regionsOnServer = admin.getRegions(clusterRegionServers.get(i)).get(); - if (regionsOnServer.size() > 0) { - serversToDecommssion.put(clusterRegionServers.get(i), regionsOnServer); + if (!regionsOnServer.isEmpty()) { + serversToDecommission.put(clusterRegionServers.get(i), regionsOnServer); break; } } @@ -74,13 +74,13 @@ public void testAsyncDecommissionRegionServers() throws Exception { ServerName remainingServer = clusterRegionServers.get(0); // Decommission - admin.decommissionRegionServers(new ArrayList(serversToDecommssion.keySet()), true) + admin.decommissionRegionServers(new ArrayList(serversToDecommission.keySet()), true) .get(); assertEquals(1, admin.listDecommissionedRegionServers().get().size()); // Verify the regions have been off the decommissioned servers, all on the remaining server. - for (ServerName server : serversToDecommssion.keySet()) { - for (RegionInfo region : serversToDecommssion.get(server)) { + for (ServerName server : serversToDecommission.keySet()) { + for (RegionInfo region : serversToDecommission.get(server)) { TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); } } @@ -91,17 +91,78 @@ public void testAsyncDecommissionRegionServers() throws Exception { TEST_UTIL.waitUntilNoRegionsInTransition(10000); // Recommission and load regions - for (ServerName server : serversToDecommssion.keySet()) { - List encodedRegionNames = serversToDecommssion.get(server).stream() - .map(region -> region.getEncodedNameAsBytes()).collect(Collectors.toList()); + for (ServerName server : serversToDecommission.keySet()) { + List encodedRegionNames = serversToDecommission.get(server).stream() + .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); admin.recommissionRegionServer(server, encodedRegionNames).get(); } assertTrue(admin.listDecommissionedRegionServers().get().isEmpty()); // Verify the regions have been moved to the recommissioned servers - for (ServerName server : serversToDecommssion.keySet()) { - for (RegionInfo region : serversToDecommssion.get(server)) { + for (ServerName server : serversToDecommission.keySet()) { + for (RegionInfo region : serversToDecommission.get(server)) { TEST_UTIL.assertRegionOnServer(region, server, 10000); } } } + + @Test + public void testAsyncDecommissionRegionServersHostsPermanently() throws Exception { + admin.balancerSwitch(false, true); + List decommissionedRegionServers = admin.listDecommissionedRegionServers().get(); + assertTrue(decommissionedRegionServers.isEmpty()); + + TEST_UTIL.createMultiRegionTable(tableName, FAMILY, 4); + + ArrayList clusterRegionServers = new ArrayList<>(admin + .getClusterMetrics(EnumSet.of(Option.LIVE_SERVERS)).get().getLiveServerMetrics().keySet()); + + assertEquals(TEST_UTIL.getHBaseCluster().getLiveRegionServerThreads().size(), + clusterRegionServers.size()); + + HashMap> serversToDecommission = new HashMap<>(); + // Get a server that has regions. We will decommission one of the servers, + // leaving one online. + int i; + for (i = 0; i < clusterRegionServers.size(); i++) { + List regionsOnServer = admin.getRegions(clusterRegionServers.get(i)).get(); + if (!regionsOnServer.isEmpty()) { + serversToDecommission.put(clusterRegionServers.get(i), regionsOnServer); + break; + } + } + + clusterRegionServers.remove(i); + ServerName remainingServer = clusterRegionServers.get(0); + + // Decommission the server permanently, setting the `matchHostNameOnly` argument to `true` + boolean matchHostNameOnly = true; + admin.decommissionRegionServers(new ArrayList(serversToDecommission.keySet()), true, + matchHostNameOnly).get(); + assertEquals(1, admin.listDecommissionedRegionServers().get().size()); + + // Verify the regions have been off the decommissioned servers, all on the remaining server. + for (ServerName server : serversToDecommission.keySet()) { + for (RegionInfo region : serversToDecommission.get(server)) { + TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); + } + } + + // Maybe the TRSP is still not finished at master side, since the reportRegionTransition just + // updates the procedure store, and we still need to wake up the procedure and execute it in the + // procedure executor, which is asynchronous + TEST_UTIL.waitUntilNoRegionsInTransition(10000); + + // Try to recommission the server and assert that the server is still decommissioned + for (ServerName server : serversToDecommission.keySet()) { + List encodedRegionNames = serversToDecommission.get(server).stream() + .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); + admin.recommissionRegionServer(server, encodedRegionNames).get(); + } + assertEquals(1, admin.listDecommissionedRegionServers().get().size()); + + // Verify that the still decommissioned servers have no regions + for (ServerName server : serversToDecommission.keySet()) { + assertTrue(serversToDecommission.get(server).isEmpty()); + } + } } From 589a0c3aa095dc624ddf4bae75282c7c678cfe87 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 19:02:35 +0100 Subject: [PATCH 13/16] Implemented a new parameterized test for the decommission API by hostname --- .../hadoop/hbase/client/TestAdmin4.java | 70 +++++++++++++++++++ .../client/TestAsyncDecommissionAdminApi.java | 49 +++++++++++-- 2 files changed, 112 insertions(+), 7 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java index e52d8ee92c3c..031d4623142f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java @@ -27,12 +27,18 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.EnumSet; +import java.util.HashMap; import java.util.List; +import java.util.stream.Collectors; +import org.apache.hadoop.hbase.ClusterMetrics; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.zookeeper.ZKUtil; import org.apache.hadoop.hbase.zookeeper.ZKWatcher; import org.apache.hadoop.hbase.zookeeper.ZNodePaths; @@ -70,6 +76,70 @@ public void testDecommissionAndStopRegionServers() throws Exception { ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, serverName.getServerName()))); } + /** + * TestCase for HBASE-28342 + */ + @Test + public void testDecommissionRegionServersPermanently() throws Exception { + List decommissionedRegionServers = ADMIN.listDecommissionedRegionServers(); + assertTrue(decommissionedRegionServers.isEmpty()); + + final TableName tableName = TableName.valueOf(name.getMethodName()); + TEST_UTIL.createMultiRegionTable(tableName, Bytes.toBytes("f"), 6); + + ArrayList clusterRegionServers = + new ArrayList<>(ADMIN.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.LIVE_SERVERS)) + .getLiveServerMetrics().keySet()); + + assertEquals(3, clusterRegionServers.size()); + + HashMap> serversToDecommission = new HashMap<>(); + // Get a server that has meta online. We will decommission two of the servers, + // leaving one online. + int i; + for (i = 0; i < clusterRegionServers.size(); i++) { + List regionsOnServer = ADMIN.getRegions(clusterRegionServers.get(i)); + if ( + ADMIN.getRegions(clusterRegionServers.get(i)).stream().anyMatch(RegionInfo::isMetaRegion) + ) { + serversToDecommission.put(clusterRegionServers.get(i), regionsOnServer); + break; + } + } + + clusterRegionServers.remove(i); + // Get another server to decommission. + serversToDecommission.put(clusterRegionServers.get(0), + ADMIN.getRegions(clusterRegionServers.get(0))); + + ServerName remainingServer = clusterRegionServers.get(1); + + // Decommission the servers with `matchHostNameOnly` set to `true` so that the hostnames are + // always maintained as decommissioned/drained + boolean matchHostNameOnly = true; + ADMIN.decommissionRegionServers(new ArrayList(serversToDecommission.keySet()), true, + matchHostNameOnly); + assertEquals(2, ADMIN.listDecommissionedRegionServers().size()); + + // Verify the regions have been off the decommissioned servers, all on the one + // remaining server. + for (ServerName server : serversToDecommission.keySet()) { + for (RegionInfo region : serversToDecommission.get(server)) { + TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); + } + } + + // Try to recommission the servers and assert that they remain decommissioned + // No regions should be loaded on them + for (ServerName server : serversToDecommission.keySet()) { + List encodedRegionNames = serversToDecommission.get(server).stream() + .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); + ADMIN.recommissionRegionServer(server, encodedRegionNames); + } + // Assert that the number of decommissioned servers is still 2! + assertEquals(2, ADMIN.listDecommissionedRegionServers().size()); + } + @Test public void testReplicationPeerModificationSwitch() throws Exception { assertTrue(ADMIN.isReplicationPeerModificationEnabled()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java index dfdb92ac1227..79fb4a2f596f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java @@ -20,16 +20,24 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import java.io.IOException; import java.util.ArrayList; import java.util.EnumSet; import java.util.HashMap; import java.util.List; +import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; import org.apache.hadoop.hbase.ClusterMetrics.Option; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.zookeeper.ZKUtil; +import org.apache.hadoop.hbase.zookeeper.ZKWatcher; +import org.apache.hadoop.hbase.zookeeper.ZNodePaths; +import org.apache.zookeeper.KeeperException; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -106,7 +114,7 @@ public void testAsyncDecommissionRegionServers() throws Exception { } @Test - public void testAsyncDecommissionRegionServersHostsPermanently() throws Exception { + public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Exception { admin.balancerSwitch(false, true); List decommissionedRegionServers = admin.listDecommissionedRegionServers().get(); assertTrue(decommissionedRegionServers.isEmpty()); @@ -136,7 +144,7 @@ public void testAsyncDecommissionRegionServersHostsPermanently() throws Exceptio // Decommission the server permanently, setting the `matchHostNameOnly` argument to `true` boolean matchHostNameOnly = true; - admin.decommissionRegionServers(new ArrayList(serversToDecommission.keySet()), true, + admin.decommissionRegionServers(new ArrayList<>(serversToDecommission.keySet()), true, matchHostNameOnly).get(); assertEquals(1, admin.listDecommissionedRegionServers().get().size()); @@ -153,16 +161,43 @@ public void testAsyncDecommissionRegionServersHostsPermanently() throws Exceptio TEST_UTIL.waitUntilNoRegionsInTransition(10000); // Try to recommission the server and assert that the server is still decommissioned + recommissionRegionServers(serversToDecommission); + assertEquals(1, admin.listDecommissionedRegionServers().get().size()); + + // Verify that the regions still belong to the remainingServer and not the decommissioned ones for (ServerName server : serversToDecommission.keySet()) { - List encodedRegionNames = serversToDecommission.get(server).stream() + for (RegionInfo region : serversToDecommission.get(server)) { + TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); + } + } + + // Clean-up ZooKeeper's state and recommission all servers for the next parameterized test run + removeServersBinaryData(serversToDecommission.keySet()); + recommissionRegionServers(serversToDecommission); + } + + private void + recommissionRegionServers(HashMap> decommissionedServers) + throws ExecutionException, InterruptedException { + for (ServerName server : decommissionedServers.keySet()) { + List encodedRegionNames = decommissionedServers.get(server).stream() .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); admin.recommissionRegionServer(server, encodedRegionNames).get(); } - assertEquals(1, admin.listDecommissionedRegionServers().get().size()); + } - // Verify that the still decommissioned servers have no regions - for (ServerName server : serversToDecommission.keySet()) { - assertTrue(serversToDecommission.get(server).isEmpty()); + private void removeServersBinaryData(Set decommissionedServers) throws IOException { + ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); + for (ServerName serverName : decommissionedServers) { + String znodePath = + ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, serverName.getServerName()); + byte[] newData = ZooKeeperProtos.DrainedZNodeServerData.newBuilder() + .setMatchHostNameOnly(false).build().toByteArray(); + try { + ZKUtil.setData(zkw, znodePath, newData); + } catch (KeeperException e) { + throw new RuntimeException(e); + } } } } From e183573dac9967bd816ca83406f6f98ec53adf40 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 12 Feb 2024 19:11:27 +0100 Subject: [PATCH 14/16] Refactored failing tests in TestAdmin4.java --- .../hadoop/hbase/client/TestAdmin4.java | 53 +++++++++++++++---- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java index 031d4623142f..55da73b12b4a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java @@ -30,18 +30,21 @@ import java.util.EnumSet; import java.util.HashMap; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import org.apache.hadoop.hbase.ClusterMetrics; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.zookeeper.ZKUtil; import org.apache.hadoop.hbase.zookeeper.ZKWatcher; import org.apache.hadoop.hbase.zookeeper.ZNodePaths; +import org.apache.zookeeper.KeeperException; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -59,7 +62,7 @@ public void testDecommissionAndStopRegionServers() throws Exception { ArrayList clusterRegionServers = new ArrayList<>(ADMIN.getRegionServers(true)); - List serversToDecommission = new ArrayList(); + List serversToDecommission = new ArrayList<>(); serversToDecommission.add(clusterRegionServers.get(0)); // Decommission @@ -80,7 +83,7 @@ public void testDecommissionAndStopRegionServers() throws Exception { * TestCase for HBASE-28342 */ @Test - public void testDecommissionRegionServersPermanently() throws Exception { + public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Exception { List decommissionedRegionServers = ADMIN.listDecommissionedRegionServers(); assertTrue(decommissionedRegionServers.isEmpty()); @@ -91,8 +94,6 @@ public void testDecommissionRegionServersPermanently() throws Exception { new ArrayList<>(ADMIN.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.LIVE_SERVERS)) .getLiveServerMetrics().keySet()); - assertEquals(3, clusterRegionServers.size()); - HashMap> serversToDecommission = new HashMap<>(); // Get a server that has meta online. We will decommission two of the servers, // leaving one online. @@ -117,7 +118,7 @@ public void testDecommissionRegionServersPermanently() throws Exception { // Decommission the servers with `matchHostNameOnly` set to `true` so that the hostnames are // always maintained as decommissioned/drained boolean matchHostNameOnly = true; - ADMIN.decommissionRegionServers(new ArrayList(serversToDecommission.keySet()), true, + ADMIN.decommissionRegionServers(new ArrayList<>(serversToDecommission.keySet()), true, matchHostNameOnly); assertEquals(2, ADMIN.listDecommissionedRegionServers().size()); @@ -131,13 +132,20 @@ public void testDecommissionRegionServersPermanently() throws Exception { // Try to recommission the servers and assert that they remain decommissioned // No regions should be loaded on them - for (ServerName server : serversToDecommission.keySet()) { - List encodedRegionNames = serversToDecommission.get(server).stream() - .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); - ADMIN.recommissionRegionServer(server, encodedRegionNames); - } + recommissionRegionServers(serversToDecommission); // Assert that the number of decommissioned servers is still 2! assertEquals(2, ADMIN.listDecommissionedRegionServers().size()); + + // Verify that all regions are still on the remainingServer and not on the decommissioned servers + for (ServerName server : serversToDecommission.keySet()) { + for (RegionInfo region : serversToDecommission.get(server)) { + TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); + } + } + + // Cleanup ZooKeeper's state and recommission all servers for the rest of tests + removeServersBinaryData(serversToDecommission.keySet()); + recommissionRegionServers(serversToDecommission); } @Test @@ -159,4 +167,29 @@ public void testReplicationPeerModificationSwitch() throws Exception { ADMIN.replicationPeerModificationSwitch(true); } } + + private void + recommissionRegionServers(HashMap> decommissionedServers) + throws IOException { + for (ServerName server : decommissionedServers.keySet()) { + List encodedRegionNames = decommissionedServers.get(server).stream() + .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); + ADMIN.recommissionRegionServer(server, encodedRegionNames); + } + } + + private void removeServersBinaryData(Set decommissionedServers) throws IOException { + ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); + for (ServerName serverName : decommissionedServers) { + String znodePath = + ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, serverName.getServerName()); + byte[] newData = ZooKeeperProtos.DrainedZNodeServerData.newBuilder() + .setMatchHostNameOnly(false).build().toByteArray(); + try { + ZKUtil.setData(zkw, znodePath, newData); + } catch (KeeperException e) { + throw new RuntimeException(e); + } + } + } } From 30c28d6bb39833ee8f31a215d5875f2cb11c8ec4 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 13 Feb 2024 10:52:26 +0100 Subject: [PATCH 15/16] Added more tests to cover the ZKNode state --- .../hadoop/hbase/client/TestAdmin4.java | 83 +++++++++++++++++-- 1 file changed, 74 insertions(+), 9 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java index 55da73b12b4a..b0c4ed248f12 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java @@ -26,18 +26,14 @@ import static org.junit.Assert.assertTrue; import java.io.IOException; -import java.util.ArrayList; -import java.util.EnumSet; -import java.util.HashMap; -import java.util.List; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; import org.apache.hadoop.hbase.ClusterMetrics; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; -import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -144,10 +140,79 @@ public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Ex } // Cleanup ZooKeeper's state and recommission all servers for the rest of tests - removeServersBinaryData(serversToDecommission.keySet()); + markZnodeAsRecommissionable(serversToDecommission.keySet()); recommissionRegionServers(serversToDecommission); } + /** + * TestCase for HBASE-28342 + */ + @Test + public void testAsyncDecommissionRegionServersSetsHostNameMatchAsFalseInZooKeeper() throws Exception { + assertTrue(ADMIN.listDecommissionedRegionServers().isEmpty()); + + ArrayList clusterRegionServers = + new ArrayList<>(ADMIN.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.LIVE_SERVERS)) + .getLiveServerMetrics().keySet()); + + ServerName decommissionedRegionServer = clusterRegionServers.get(0); + clusterRegionServers.remove(0); + + // Decommission the servers with `matchHostNameOnly` set to `false` so that the hostnames are + // not always considered as decommissioned/drained + boolean expectedMatchHostNameOnly = false; + ADMIN.decommissionRegionServers(Collections.singletonList(decommissionedRegionServer), true, + expectedMatchHostNameOnly); + assertEquals(1, ADMIN.listDecommissionedRegionServers().size()); + + // Read the node's data in ZooKeeper and assert that it was set as expected + ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); + String znodePath =ZNodePaths.joinZNode( + zkw.getZNodePaths().drainingZNode, decommissionedRegionServer.getServerName() + ); + DrainedZNodeServerData data = DrainedZNodeServerData.parseFrom(ZKUtil.getData(zkw, znodePath)); + assertEquals(expectedMatchHostNameOnly, data.getMatchHostNameOnly()); + + // Recommission the server + ADMIN.recommissionRegionServer(decommissionedRegionServer, new ArrayList<>()); + assertEquals(0, ADMIN.listDecommissionedRegionServers().size()); + } + + /** + * TestCase for HBASE-28342 + */ + @Test + public void testAsyncDecommissionRegionServersSetsHostNameMatchAsTrueInZooKeeper() throws Exception { + assertTrue(ADMIN.listDecommissionedRegionServers().isEmpty()); + + ArrayList clusterRegionServers = + new ArrayList<>(ADMIN.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.LIVE_SERVERS)) + .getLiveServerMetrics().keySet()); + + ServerName decommissionedRegionServer = clusterRegionServers.get(0); + clusterRegionServers.remove(0); + + // Decommission the servers with `matchHostNameOnly` set to `true` so that the hostnames are + // always considered as decommissioned/drained + boolean expectedMatchHostNameOnly = true; + ADMIN.decommissionRegionServers(Collections.singletonList(decommissionedRegionServer), true, + expectedMatchHostNameOnly); + assertEquals(1, ADMIN.listDecommissionedRegionServers().size()); + + // Read the node's data in ZooKeeper and assert that it was set as expected + ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); + String znodePath =ZNodePaths.joinZNode( + zkw.getZNodePaths().drainingZNode, decommissionedRegionServer.getServerName() + ); + DrainedZNodeServerData data = DrainedZNodeServerData.parseFrom(ZKUtil.getData(zkw, znodePath)); + assertEquals(expectedMatchHostNameOnly, data.getMatchHostNameOnly()); + + // Reset the node's data in ZooKeeper in order to be able to recommission the server + markZnodeAsRecommissionable(Collections.singleton(decommissionedRegionServer)); + ADMIN.recommissionRegionServer(decommissionedRegionServer, new ArrayList<>()); + assertEquals(0, ADMIN.listDecommissionedRegionServers().size()); + } + @Test public void testReplicationPeerModificationSwitch() throws Exception { assertTrue(ADMIN.isReplicationPeerModificationEnabled()); @@ -178,12 +243,12 @@ public void testReplicationPeerModificationSwitch() throws Exception { } } - private void removeServersBinaryData(Set decommissionedServers) throws IOException { + private void markZnodeAsRecommissionable(Set decommissionedServers) throws IOException { ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); for (ServerName serverName : decommissionedServers) { String znodePath = ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, serverName.getServerName()); - byte[] newData = ZooKeeperProtos.DrainedZNodeServerData.newBuilder() + byte[] newData = DrainedZNodeServerData.newBuilder() .setMatchHostNameOnly(false).build().toByteArray(); try { ZKUtil.setData(zkw, znodePath, newData); From 2f32364b2e252b596b5b3fd0a2d31f03f5221b0a Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 13 Feb 2024 10:55:36 +0100 Subject: [PATCH 16/16] Fixed spotless issues. --- .../hadoop/hbase/client/TestAdmin4.java | 34 ++++++++++--------- .../client/TestAsyncDecommissionAdminApi.java | 7 ++-- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java index b0c4ed248f12..b6ce94d72b9d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin4.java @@ -33,7 +33,6 @@ import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; -import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -45,6 +44,8 @@ import org.junit.Test; import org.junit.experimental.categories.Category; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos.DrainedZNodeServerData; + @Category({ MediumTests.class, ClientTests.class }) public class TestAdmin4 extends TestAdminBase { @ClassRule @@ -132,7 +133,8 @@ public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Ex // Assert that the number of decommissioned servers is still 2! assertEquals(2, ADMIN.listDecommissionedRegionServers().size()); - // Verify that all regions are still on the remainingServer and not on the decommissioned servers + // Verify that all regions are still on the remainingServer and not on the decommissioned + // servers for (ServerName server : serversToDecommission.keySet()) { for (RegionInfo region : serversToDecommission.get(server)) { TEST_UTIL.assertRegionOnServer(region, remainingServer, 10000); @@ -148,7 +150,8 @@ public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Ex * TestCase for HBASE-28342 */ @Test - public void testAsyncDecommissionRegionServersSetsHostNameMatchAsFalseInZooKeeper() throws Exception { + public void testAsyncDecommissionRegionServersSetsHostNameMatchDataFalseInZooKeeperAsExpected() + throws Exception { assertTrue(ADMIN.listDecommissionedRegionServers().isEmpty()); ArrayList clusterRegionServers = @@ -167,9 +170,8 @@ public void testAsyncDecommissionRegionServersSetsHostNameMatchAsFalseInZooKeepe // Read the node's data in ZooKeeper and assert that it was set as expected ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); - String znodePath =ZNodePaths.joinZNode( - zkw.getZNodePaths().drainingZNode, decommissionedRegionServer.getServerName() - ); + String znodePath = ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, + decommissionedRegionServer.getServerName()); DrainedZNodeServerData data = DrainedZNodeServerData.parseFrom(ZKUtil.getData(zkw, znodePath)); assertEquals(expectedMatchHostNameOnly, data.getMatchHostNameOnly()); @@ -182,7 +184,8 @@ public void testAsyncDecommissionRegionServersSetsHostNameMatchAsFalseInZooKeepe * TestCase for HBASE-28342 */ @Test - public void testAsyncDecommissionRegionServersSetsHostNameMatchAsTrueInZooKeeper() throws Exception { + public void testAsyncDecommissionRegionServersSetsHostNameMatchToTrueInZooKeeperAsExpected() + throws Exception { assertTrue(ADMIN.listDecommissionedRegionServers().isEmpty()); ArrayList clusterRegionServers = @@ -201,9 +204,8 @@ public void testAsyncDecommissionRegionServersSetsHostNameMatchAsTrueInZooKeeper // Read the node's data in ZooKeeper and assert that it was set as expected ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); - String znodePath =ZNodePaths.joinZNode( - zkw.getZNodePaths().drainingZNode, decommissionedRegionServer.getServerName() - ); + String znodePath = ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, + decommissionedRegionServer.getServerName()); DrainedZNodeServerData data = DrainedZNodeServerData.parseFrom(ZKUtil.getData(zkw, znodePath)); assertEquals(expectedMatchHostNameOnly, data.getMatchHostNameOnly()); @@ -233,9 +235,8 @@ public void testReplicationPeerModificationSwitch() throws Exception { } } - private void - recommissionRegionServers(HashMap> decommissionedServers) - throws IOException { + private void recommissionRegionServers( + HashMap> decommissionedServers) throws IOException { for (ServerName server : decommissionedServers.keySet()) { List encodedRegionNames = decommissionedServers.get(server).stream() .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList()); @@ -243,13 +244,14 @@ public void testReplicationPeerModificationSwitch() throws Exception { } } - private void markZnodeAsRecommissionable(Set decommissionedServers) throws IOException { + private void markZnodeAsRecommissionable(Set decommissionedServers) + throws IOException { ZKWatcher zkw = TEST_UTIL.getZooKeeperWatcher(); for (ServerName serverName : decommissionedServers) { String znodePath = ZNodePaths.joinZNode(zkw.getZNodePaths().drainingZNode, serverName.getServerName()); - byte[] newData = DrainedZNodeServerData.newBuilder() - .setMatchHostNameOnly(false).build().toByteArray(); + byte[] newData = + DrainedZNodeServerData.newBuilder().setMatchHostNameOnly(false).build().toByteArray(); try { ZKUtil.setData(zkw, znodePath, newData); } catch (KeeperException e) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java index 79fb4a2f596f..2834df3f383f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncDecommissionAdminApi.java @@ -31,7 +31,6 @@ import org.apache.hadoop.hbase.ClusterMetrics.Option; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; -import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.zookeeper.ZKUtil; @@ -44,6 +43,8 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ZooKeeperProtos; + @RunWith(Parameterized.class) @Category({ ClientTests.class, MediumTests.class }) public class TestAsyncDecommissionAdminApi extends TestAsyncAdminBase { @@ -177,8 +178,8 @@ public void testAsyncDecommissionRegionServersByHostNamesPermanently() throws Ex } private void - recommissionRegionServers(HashMap> decommissionedServers) - throws ExecutionException, InterruptedException { + recommissionRegionServers(HashMap> decommissionedServers) + throws ExecutionException, InterruptedException { for (ServerName server : decommissionedServers.keySet()) { List encodedRegionNames = decommissionedServers.get(server).stream() .map(RegionInfo::getEncodedNameAsBytes).collect(Collectors.toList());