From 2b1b51e720212c4baa41cdfd4e3266815c262859 Mon Sep 17 00:00:00 2001 From: belugabehr <12578579+belugabehr@users.noreply.github.com> Date: Sun, 17 Nov 2019 05:13:52 -0500 Subject: [PATCH 1/2] HBASE-23102: Improper Usage of Map putIfAbsent (#828) Signed-off-by: Wellington Chevreuil --- .../hbase/procedure2/ProcedureExecutor.java | 8 +++-- .../procedure2/RemoteProcedureDispatcher.java | 3 +- .../hbase/rsgroup/RSGroupAdminServer.java | 17 +++++---- .../hbase/executor/ExecutorService.java | 24 ++++++------- .../hbase/master/RegionsRecoveryChore.java | 4 +-- .../hbase/master/assignment/RegionStates.java | 23 +++--------- .../FileArchiverNotifierFactoryImpl.java | 10 ++---- .../hadoop/hbase/quotas/QuotaCache.java | 36 +++++++++---------- .../throttle/StoreHotnessProtector.java | 5 +-- 9 files changed, 56 insertions(+), 74 deletions(-) diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java index 4e7c1ca04a73..f6263a5530bb 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java @@ -836,10 +836,12 @@ public void setFailureResultForNonce(NonceKey nonceKey, String procName, User pr return; } - Procedure proc = - new FailedProcedure<>(procId.longValue(), procName, procOwner, nonceKey, exception); + completed.computeIfAbsent(procId, (key) -> { + Procedure proc = + new FailedProcedure<>(procId.longValue(), procName, procOwner, nonceKey, exception); - completed.putIfAbsent(procId, new CompletedProcedureRetainer<>(proc)); + return new CompletedProcedureRetainer<>(proc); + }); } // ========================================================================== diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java index b880043c0164..b58a875b12de 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java @@ -152,8 +152,7 @@ public void join() { */ public void addNode(final TRemote key) { assert key != null : "Tried to add a node with a null key"; - final BufferNode newNode = new BufferNode(key); - nodeMap.putIfAbsent(key, newNode); + nodeMap.computeIfAbsent(key, k -> new BufferNode(k)); } /** diff --git a/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java b/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java index bf535188c5bf..9d1b03e18eba 100644 --- a/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java +++ b/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java @@ -694,9 +694,8 @@ Map>> getRSGroupAssignmentsByTable( if (currRegion.isSplitParent()) { continue; } - assignments.putIfAbsent(currTable, new HashMap<>()); - assignments.get(currTable).putIfAbsent(currServer, new ArrayList<>()); - assignments.get(currTable).get(currServer).add(currRegion); + assignments.computeIfAbsent(currTable, key -> new HashMap<>()) + .computeIfAbsent(currServer, key -> new ArrayList<>()).add(currRegion); } } @@ -710,10 +709,14 @@ Map>> getRSGroupAssignmentsByTable( // add all tables that are members of the group for (TableName tableName : rsGroupInfo.getTables()) { if (assignments.containsKey(tableName)) { - result.put(tableName, new HashMap<>()); - result.get(tableName).putAll(serverMap); - result.get(tableName).putAll(assignments.get(tableName)); - LOG.debug("Adding assignments for {}: {}", tableName, assignments.get(tableName)); + Map> tableResults = new HashMap<>(serverMap); + + Map> tableAssignments = assignments.get(tableName); + tableResults.putAll(tableAssignments); + + result.put(tableName, tableResults); + + LOG.debug("Adding assignments for {}: {}", tableName, tableAssignments); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java index 5ddd9316cd33..84817bad161c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java @@ -60,7 +60,7 @@ public class ExecutorService { private static final Logger LOG = LoggerFactory.getLogger(ExecutorService.class); // hold the all the executors created in a map addressable by their names - private final ConcurrentHashMap executorMap = new ConcurrentHashMap<>(); + private final ConcurrentMap executorMap = new ConcurrentHashMap<>(); // Name of the server hosting this executor service. private final String servername; @@ -84,18 +84,16 @@ public ExecutorService(final String servername) { */ public void startExecutorService(final ExecutorConfig config) { final String name = config.getName(); - if (this.executorMap.get(name) != null) { - throw new RuntimeException( - "An executor service with the name " + name + " is already running!"); - } - Executor hbes = new Executor(config); - if (this.executorMap.putIfAbsent(name, hbes) != null) { - throw new RuntimeException( - "An executor service with the name " + name + " is already running (2)!"); - } - LOG.debug("Starting executor service name=" + name + ", corePoolSize=" - + hbes.threadPoolExecutor.getCorePoolSize() + ", maxPoolSize=" - + hbes.threadPoolExecutor.getMaximumPoolSize()); + Executor hbes = this.executorMap.compute(name, (key, value) -> { + if (value != null) { + throw new RuntimeException( + "An executor service with the name " + key + " is already running!"); + } + return new Executor(config); + }); + + LOG.debug("Starting executor service name={}, corePoolSize={}, maxPoolSize={}", name, + hbes.threadPoolExecutor.getCorePoolSize(), hbes.threadPoolExecutor.getMaximumPoolSize()); } boolean isExecutorServiceRunning(String name) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionsRecoveryChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionsRecoveryChore.java index f6ddeae7c390..c88516f2ffe4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionsRecoveryChore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionsRecoveryChore.java @@ -155,8 +155,6 @@ private void prepareTableToReopenRegionsMap( } LOG.warn("Region {} for Table {} has high storeFileRefCount {}, considering it for reopen..", regionInfo.getRegionNameAsString(), tableName, regionStoreRefCount); - tableToReopenRegionsMap.putIfAbsent(tableName, new ArrayList<>()); - tableToReopenRegionsMap.get(tableName).add(regionName); - + tableToReopenRegionsMap.computeIfAbsent(tableName, (key) -> new ArrayList<>()).add(regionName); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java index 3053aece59d4..1ee326e044cf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java @@ -113,9 +113,8 @@ public boolean isRegionInRegionStates(final RegionInfo hri) { // RegionStateNode helpers // ========================================================================== RegionStateNode createRegionStateNode(RegionInfo regionInfo) { - RegionStateNode newNode = new RegionStateNode(regionInfo, regionInTransition); - RegionStateNode oldNode = regionsMap.putIfAbsent(regionInfo.getRegionName(), newNode); - return oldNode != null ? oldNode : newNode; + return regionsMap.computeIfAbsent(regionInfo.getRegionName(), + key -> new RegionStateNode(regionInfo, regionInTransition)); } public RegionStateNode getOrCreateRegionStateNode(RegionInfo regionInfo) { @@ -583,7 +582,7 @@ public ServerName getRegionServerOfRegion(RegionInfo regionInfo) { } // Add online servers with no assignment for the table. for (Map> table : result.values()) { - for (ServerName serverName : onlineServers) { + for (ServerName serverName : serverMap.keySet()) { table.computeIfAbsent(serverName, key -> new ArrayList<>()); } } @@ -703,13 +702,7 @@ public Exception getException() { public RegionFailedOpen addToFailedOpen(final RegionStateNode regionNode) { final byte[] key = regionNode.getRegionInfo().getRegionName(); - RegionFailedOpen node = regionFailedOpen.get(key); - if (node == null) { - RegionFailedOpen newNode = new RegionFailedOpen(regionNode); - RegionFailedOpen oldNode = regionFailedOpen.putIfAbsent(key, newNode); - node = oldNode != null ? oldNode : newNode; - } - return node; + return regionFailedOpen.computeIfAbsent(key, (k) -> new RegionFailedOpen(regionNode)); } public RegionFailedOpen getFailedOpen(final RegionInfo regionInfo) { @@ -740,13 +733,7 @@ public List getRegionFailedOpen() { * where we can. */ public ServerStateNode getOrCreateServer(final ServerName serverName) { - ServerStateNode node = serverMap.get(serverName); - if (node == null) { - node = new ServerStateNode(serverName); - ServerStateNode oldNode = serverMap.putIfAbsent(serverName, node); - node = oldNode != null ? oldNode : node; - } - return node; + return serverMap.computeIfAbsent(serverName, key -> new ServerStateNode(key)); } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileArchiverNotifierFactoryImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileArchiverNotifierFactoryImpl.java index 9168694ac01a..58307f3a7358 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileArchiverNotifierFactoryImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileArchiverNotifierFactoryImpl.java @@ -19,6 +19,7 @@ import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import org.apache.commons.lang3.builder.HashCodeBuilder; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; @@ -34,7 +35,7 @@ public final class FileArchiverNotifierFactoryImpl implements FileArchiverNotifi private static final FileArchiverNotifierFactoryImpl DEFAULT_INSTANCE = new FileArchiverNotifierFactoryImpl(); private static volatile FileArchiverNotifierFactory CURRENT_INSTANCE = DEFAULT_INSTANCE; - private final ConcurrentHashMap CACHE; + private final ConcurrentMap CACHE; private FileArchiverNotifierFactoryImpl() { CACHE = new ConcurrentHashMap<>(); @@ -60,12 +61,7 @@ static void reset() { public FileArchiverNotifier get(Connection conn, Configuration conf, FileSystem fs, TableName tn) { // Ensure that only one instance is exposed to callers - final FileArchiverNotifier newMapping = new FileArchiverNotifierImpl(conn, conf, fs, tn); - final FileArchiverNotifier previousMapping = CACHE.putIfAbsent(tn, newMapping); - if (previousMapping == null) { - return newMapping; - } - return previousMapping; + return CACHE.computeIfAbsent(tn, key -> new FileArchiverNotifierImpl(conn, conf, fs, key)); } public int getCacheSize() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java index c8839ad61d1b..56253f7fcbb2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java @@ -26,6 +26,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.ClusterMetrics; import org.apache.hadoop.hbase.ClusterMetrics.Option; @@ -62,13 +63,10 @@ public class QuotaCache implements Stoppable { // for testing purpose only, enforce the cache to be always refreshed static boolean TEST_FORCE_REFRESH = false; - private final ConcurrentHashMap namespaceQuotaCache = - new ConcurrentHashMap<>(); - private final ConcurrentHashMap tableQuotaCache = - new ConcurrentHashMap<>(); - private final ConcurrentHashMap userQuotaCache = - new ConcurrentHashMap<>(); - private final ConcurrentHashMap regionServerQuotaCache = + private final ConcurrentMap namespaceQuotaCache = new ConcurrentHashMap<>(); + private final ConcurrentMap tableQuotaCache = new ConcurrentHashMap<>(); + private final ConcurrentMap userQuotaCache = new ConcurrentHashMap<>(); + private final ConcurrentMap regionServerQuotaCache = new ConcurrentHashMap<>(); private volatile boolean exceedThrottleQuotaEnabled = false; // factors used to divide cluster scope quota into machine scope quota @@ -166,8 +164,7 @@ protected boolean isExceedThrottleQuotaEnabled() { * Returns the QuotaState requested. If the quota info is not in cache an empty one will be * returned and the quota request will be enqueued for the next cache refresh. */ - private QuotaState getQuotaState(final ConcurrentHashMap quotasMap, - final K key) { + private QuotaState getQuotaState(final ConcurrentMap quotasMap, final K key) { return computeIfAbsent(quotasMap, key, QuotaState::new, this::triggerCacheRefresh); } @@ -209,17 +206,18 @@ public QuotaRefresherChore(final int period, final Stoppable stoppable) { protected void chore() { // Prefetch online tables/namespaces for (TableName table : ((HRegionServer) QuotaCache.this.rsServices).getOnlineTables()) { - if (table.isSystemTable()) continue; - if (!QuotaCache.this.tableQuotaCache.containsKey(table)) { - QuotaCache.this.tableQuotaCache.putIfAbsent(table, new QuotaState()); - } - String ns = table.getNamespaceAsString(); - if (!QuotaCache.this.namespaceQuotaCache.containsKey(ns)) { - QuotaCache.this.namespaceQuotaCache.putIfAbsent(ns, new QuotaState()); + if (table.isSystemTable()) { + continue; } + QuotaCache.this.tableQuotaCache.computeIfAbsent(table, key -> new QuotaState()); + + final String ns = table.getNamespaceAsString(); + + QuotaCache.this.namespaceQuotaCache.computeIfAbsent(ns, key -> new QuotaState()); } - QuotaCache.this.regionServerQuotaCache.putIfAbsent(QuotaTableUtil.QUOTA_REGION_SERVER_ROW_KEY, - new QuotaState()); + + QuotaCache.this.regionServerQuotaCache + .computeIfAbsent(QuotaTableUtil.QUOTA_REGION_SERVER_ROW_KEY, key -> new QuotaState()); updateQuotaFactors(); fetchNamespaceQuotaState(); @@ -302,7 +300,7 @@ private void fetchExceedThrottleQuota() { } private void fetch(final String type, - final ConcurrentHashMap quotasMap, final Fetcher fetcher) { + final ConcurrentMap quotasMap, final Fetcher fetcher) { long now = EnvironmentEdgeManager.currentTime(); long refreshPeriod = getPeriod(); long evictPeriod = refreshPeriod * EVICT_PERIOD_FACTOR; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/StoreHotnessProtector.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/StoreHotnessProtector.java index 1ab38adb97b3..70683cb45722 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/StoreHotnessProtector.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/StoreHotnessProtector.java @@ -19,6 +19,7 @@ import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.atomic.AtomicInteger; import org.apache.hadoop.conf.Configuration; @@ -78,7 +79,7 @@ public class StoreHotnessProtector { private final static int DEFAULT_PARALLEL_PUT_STORE_THREADS_LIMIT_MIN_COLUMN_NUM = 100; private final static int DEFAULT_PARALLEL_PREPARE_PUT_STORE_MULTIPLIER = 2; - private final Map preparePutToStoreMap = + private final ConcurrentMap preparePutToStoreMap = new ConcurrentSkipListMap<>(Bytes.BYTES_RAWCOMPARATOR); private final Region region; @@ -119,7 +120,7 @@ private static void logDisabledMessageOnce() { public void update(Configuration conf) { init(conf); preparePutToStoreMap.clear(); - LOG.debug("update config: " + toString()); + LOG.debug("update config: {}", this); } public void start(Map> familyMaps) throws RegionTooBusyException { From 1723067ab869e84f2e39dd1b7004f456e9097368 Mon Sep 17 00:00:00 2001 From: Nick Dimiduk Date: Thu, 23 Feb 2023 17:31:49 +0100 Subject: [PATCH 2/2] fix TestBalancer --- .../apache/hadoop/hbase/master/TestBalancer.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestBalancer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestBalancer.java index 507874b2f8fe..2ae7813117de 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestBalancer.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestBalancer.java @@ -17,6 +17,12 @@ */ package org.apache.hadoop.hbase.master; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.emptyIterable; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -39,12 +45,15 @@ import org.junit.Test; import org.junit.experimental.categories.Category; import org.junit.rules.TestName; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Test balancer with disabled table */ @Category({ MasterTests.class, LargeTests.class }) public class TestBalancer { + private static final Logger LOG = LoggerFactory.getLogger(TestBalancer.class); @ClassRule public static final HBaseClassTestRule CLASS_RULE = @@ -85,8 +94,10 @@ public void testAssignmentsForBalancer() throws Exception { Map>> assignments = assignmentManager.getRegionStates().getAssignmentsForBalancer(tableStateManager, serverManager.getOnlineServersList()); + assignments.forEach((k, v) -> LOG.debug("{}: {}", k, v)); assertFalse(assignments.containsKey(disableTableName)); assertTrue(assignments.containsKey(tableName)); - assertFalse(assignments.get(tableName).containsKey(sn1)); + assertThat(assignments.get(tableName), + allOf(notNullValue(), hasEntry(equalTo(sn1), emptyIterable()))); } }