From f5d018b83a2fd289cb3d4435c1f690f34a807608 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 11 Oct 2023 09:10:49 -0400 Subject: [PATCH 1/5] Make ServerManager rsAdmins map thread safe --- .../java/org/apache/hadoop/hbase/master/ServerManager.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 ed37fd954443..992eb271bcb1 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 @@ -23,12 +23,12 @@ import java.net.InetAddress; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.CopyOnWriteArrayList; @@ -128,7 +128,8 @@ public class ServerManager { * Map of admin interfaces per registered regionserver; these interfaces we use to control * regionservers out on the cluster */ - private final Map rsAdmins = new HashMap<>(); + private final Map rsAdmins = + new ConcurrentHashMap<>(); /** List of region servers that should not get any more new regions. */ private final ArrayList drainingServers = new ArrayList<>(); From a7f590c26425108292cd78b2138a6217a3ee7bf8 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Thu, 12 Oct 2023 10:01:38 -0400 Subject: [PATCH 2/5] use computeIfAbsent, use ConcurrentMap declaration --- .../hadoop/hbase/master/ServerManager.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) 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 992eb271bcb1..5761004e5f28 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 @@ -29,6 +29,7 @@ import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.CopyOnWriteArrayList; @@ -128,7 +129,7 @@ public class ServerManager { * Map of admin interfaces per registered regionserver; these interfaces we use to control * regionservers out on the cluster */ - private final Map rsAdmins = + private final ConcurrentMap rsAdmins = new ConcurrentHashMap<>(); /** List of region servers that should not get any more new regions. */ @@ -719,14 +720,19 @@ public static void closeRegionSilentlyAndWait(ClusterConnection connection, Serv public AdminService.BlockingInterface getRsAdmin(final ServerName sn) throws IOException { AdminService.BlockingInterface admin = this.rsAdmins.get(sn); if (admin == null) { - LOG.debug("New admin connection to " + sn.toString()); - if (sn.equals(master.getServerName()) && master instanceof HRegionServer) { - // A master is also a region server now, see HBASE-10569 for details - admin = ((HRegionServer) master).getRSRpcServices(); - } else { - admin = this.connection.getAdmin(sn); - } - this.rsAdmins.put(sn, admin); + return this.rsAdmins.computeIfAbsent(sn, server -> { + LOG.debug("New admin connection to " + server.toString()); + if (server.equals(master.getServerName()) && master instanceof HRegionServer) { + // A master is also a region server now, see HBASE-10569 for details + return ((HRegionServer) master).getRSRpcServices(); + } else { + try { + return this.connection.getAdmin(server); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + }); } return admin; } From 0d15ff5d687aac13841cca7d3587032537b69ca4 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Thu, 12 Oct 2023 10:14:10 -0400 Subject: [PATCH 3/5] don't use computeIfAbsent --- .../hadoop/hbase/master/ServerManager.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) 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 5761004e5f28..efa681a4ea0f 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 @@ -720,19 +720,14 @@ public static void closeRegionSilentlyAndWait(ClusterConnection connection, Serv public AdminService.BlockingInterface getRsAdmin(final ServerName sn) throws IOException { AdminService.BlockingInterface admin = this.rsAdmins.get(sn); if (admin == null) { - return this.rsAdmins.computeIfAbsent(sn, server -> { - LOG.debug("New admin connection to " + server.toString()); - if (server.equals(master.getServerName()) && master instanceof HRegionServer) { - // A master is also a region server now, see HBASE-10569 for details - return ((HRegionServer) master).getRSRpcServices(); - } else { - try { - return this.connection.getAdmin(server); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - }); + LOG.debug("New admin connection to " + sn.toString()); + if (sn.equals(master.getServerName()) && master instanceof HRegionServer) { + // A master is also a region server now, see HBASE-10569 for details + admin = ((HRegionServer) master).getRSRpcServices(); + } else { + admin = this.connection.getAdmin(sn); + } + this.rsAdmins.put(sn, admin); } return admin; } From c32e1034b6db1607dcf54e7af300bb71aaffe7a1 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 18 Oct 2023 14:50:34 -0400 Subject: [PATCH 4/5] rm rsAdmins cache --- .../hadoop/hbase/master/ServerManager.java | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) 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 efa681a4ea0f..887e5c3dc74d 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 @@ -28,8 +28,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.CopyOnWriteArrayList; @@ -125,13 +123,6 @@ public class ServerManager { private final ConcurrentNavigableMap onlineServers = new ConcurrentSkipListMap<>(); - /** - * Map of admin interfaces per registered regionserver; these interfaces we use to control - * regionservers out on the cluster - */ - private final ConcurrentMap rsAdmins = - new ConcurrentHashMap<>(); - /** List of region servers that should not get any more new regions. */ private final ArrayList drainingServers = new ArrayList<>(); @@ -404,7 +395,6 @@ public ServerName findServerWithSameHostnamePortWithLock(final ServerName server void recordNewServerWithLock(final ServerName serverName, final ServerMetrics sl) { LOG.info("Registering regionserver=" + serverName); this.onlineServers.put(serverName, sl); - this.rsAdmins.remove(serverName); } public RegionStoreSequenceIds getLastFlushedSequenceId(byte[] encodedRegionName) { @@ -606,7 +596,6 @@ public synchronized void moveFromOnlineToDeadServers(final ServerName sn) { LOG.trace("Expiration of {} but server not online", sn); } } - this.rsAdmins.remove(sn); } /* @@ -718,18 +707,13 @@ public static void closeRegionSilentlyAndWait(ClusterConnection connection, Serv * @throws RetriesExhaustedException wrapping a ConnectException if failed */ public AdminService.BlockingInterface getRsAdmin(final ServerName sn) throws IOException { - AdminService.BlockingInterface admin = this.rsAdmins.get(sn); - if (admin == null) { - LOG.debug("New admin connection to " + sn.toString()); - if (sn.equals(master.getServerName()) && master instanceof HRegionServer) { - // A master is also a region server now, see HBASE-10569 for details - admin = ((HRegionServer) master).getRSRpcServices(); - } else { - admin = this.connection.getAdmin(sn); - } - this.rsAdmins.put(sn, admin); + LOG.debug("New admin connection to " + sn.toString()); + if (sn.equals(master.getServerName()) && master instanceof HRegionServer) { + // A master is also a region server now, see HBASE-10569 for details + return ((HRegionServer) master).getRSRpcServices(); + } else { + return this.connection.getAdmin(sn); } - return admin; } /** From 9e71dd35c265a83d11fa980131e62921f7c61a01 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Fri, 20 Oct 2023 09:21:40 -0400 Subject: [PATCH 5/5] improve log format --- .../main/java/org/apache/hadoop/hbase/master/ServerManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 887e5c3dc74d..196a1a582ed8 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 @@ -707,7 +707,7 @@ public static void closeRegionSilentlyAndWait(ClusterConnection connection, Serv * @throws RetriesExhaustedException wrapping a ConnectException if failed */ public AdminService.BlockingInterface getRsAdmin(final ServerName sn) throws IOException { - LOG.debug("New admin connection to " + sn.toString()); + LOG.debug("New admin connection to {}", sn); if (sn.equals(master.getServerName()) && master instanceof HRegionServer) { // A master is also a region server now, see HBASE-10569 for details return ((HRegionServer) master).getRSRpcServices();