From f24615d932b7954aefb5d88e18fddddd48d5e65d Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Fri, 28 Oct 2022 15:11:49 +0100 Subject: [PATCH 1/6] HBASE-27474 Evict blocks on split/merge; Avoid caching reference/hlinks if compaction is enabled Change-Id: I63296f5f95c9549d75ea7c991d2202c2e550300e --- .../hbase/shaded/protobuf/ProtobufUtil.java | 14 +++- .../main/protobuf/server/region/Admin.proto | 1 + .../hbase/io/hfile/HFilePreadReader.java | 4 +- .../hbase/io/hfile/HFileReaderImpl.java | 19 ++++- .../assignment/AssignmentManagerUtil.java | 2 +- .../assignment/CloseRegionProcedure.java | 9 ++- .../TransitRegionStateProcedure.java | 18 ++++- .../procedure/RSProcedureDispatcher.java | 16 +++- .../hbase/regionserver/CompactSplit.java | 2 +- .../hbase/regionserver/HRegionServer.java | 3 + .../hbase/regionserver/RSRpcServices.java | 3 +- .../handler/OpenRegionHandler.java | 5 ++ .../handler/UnassignRegionHandler.java | 18 ++++- .../hadoop/hbase/io/hfile/TestPrefetch.java | 79 +++++++++++++++++++ 14 files changed, 175 insertions(+), 18 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java index 86e15fe3be16..afab0b7050a5 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java @@ -3005,6 +3005,18 @@ public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte[] regionName, ServerName destinationServer, long closeProcId) { + return ProtobufUtil.getBuilder(server, regionName, destinationServer, closeProcId).build(); + } + + public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte[] regionName, + ServerName destinationServer, long closeProcId, boolean evictCache) { + CloseRegionRequest.Builder builder = getBuilder(server, regionName, destinationServer, closeProcId); + builder.setEvictCache(evictCache); + return builder.build(); + } + + public static CloseRegionRequest.Builder getBuilder(ServerName server, byte[] regionName, + ServerName destinationServer, long closeProcId){ CloseRegionRequest.Builder builder = CloseRegionRequest.newBuilder(); RegionSpecifier region = RequestConverter.buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName); @@ -3016,7 +3028,7 @@ public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte builder.setServerStartCode(server.getStartcode()); } builder.setCloseProcId(closeProcId); - return builder.build(); + return builder; } public static ProcedureDescription buildProcedureDescription(String signature, String instance, diff --git a/hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto b/hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto index 89b9985e496b..cd88a0ca7cdb 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto @@ -122,6 +122,7 @@ message CloseRegionRequest { // the intended server for this RPC. optional uint64 serverStartCode = 5; optional int64 close_proc_id = 6 [default = -1]; + optional bool evict_cache = 7 [default = false]; } message CloseRegionResponse { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java index 0eb2aa7db000..440f4a7909aa 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java @@ -35,7 +35,7 @@ public HFilePreadReader(ReaderContext context, HFileInfo fileInfo, CacheConfig c Configuration conf) throws IOException { super(context, fileInfo, cacheConf, conf); // Prefetch file blocks upon open if requested - if (cacheConf.shouldPrefetchOnOpen()) { + if (cacheConf.shouldPrefetchOnOpen() && cacheIfCompactionsOff()) { PrefetchExecutor.request(path, new Runnable() { @Override public void run() { @@ -97,7 +97,7 @@ public void close(boolean evictOnClose) throws IOException { if (evictOnClose) { int numEvicted = cache.evictBlocksByHfileName(name); if (LOG.isTraceEnabled()) { - LOG.trace("On close, file=" + name + " evicted=" + numEvicted + " block(s)"); + LOG.trace("On close, file= {} evicted= {} block(s)", name, numEvicted); } } }); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java index d4595e42ac1f..fb33f6df0323 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java @@ -20,6 +20,7 @@ import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.BLOCK_CACHE_KEY_KEY; import io.opentelemetry.api.common.Attributes; +import static org.apache.hadoop.hbase.regionserver.CompactSplit.HBASE_REGION_SERVER_ENABLE_COMPACTION; import io.opentelemetry.api.trace.Span; import java.io.DataInput; import java.io.IOException; @@ -40,12 +41,15 @@ import org.apache.hadoop.hbase.SizeCachedKeyValue; import org.apache.hadoop.hbase.SizeCachedNoTagsByteBufferKeyValue; import org.apache.hadoop.hbase.SizeCachedNoTagsKeyValue; +import org.apache.hadoop.hbase.io.HFileLink; import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoder; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; import org.apache.hadoop.hbase.io.encoding.HFileBlockDecodingContext; import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.regionserver.KeyValueScanner; +import org.apache.hadoop.hbase.regionserver.StoreFileInfo; +import org.apache.hadoop.hbase.trace.TraceUtil; import org.apache.hadoop.hbase.util.ByteBufferUtils; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.IdLock; @@ -1264,6 +1268,8 @@ public HFileBlock readBlock(long dataBlockOffset, long onDiskBlockSize, final bo new BlockCacheKey(name, dataBlockOffset, this.isPrimaryReplicaReader(), expectedBlockType); Attributes attributes = Attributes.of(BLOCK_CACHE_KEY_KEY, cacheKey.toString()); + boolean cacheable = cacheBlock && cacheIfCompactionsOff(); + boolean useLock = false; IdLock.Entry lockEntry = null; final Span span = Span.current(); @@ -1305,7 +1311,7 @@ public HFileBlock readBlock(long dataBlockOffset, long onDiskBlockSize, final bo return cachedBlock; } - if (!useLock && cacheBlock && cacheConf.shouldLockOnCacheMiss(expectedBlockType)) { + if (!useLock && cacheable && cacheConf.shouldLockOnCacheMiss(expectedBlockType)) { // check cache again with lock useLock = true; continue; @@ -1324,10 +1330,10 @@ public HFileBlock readBlock(long dataBlockOffset, long onDiskBlockSize, final bo // Don't need the unpacked block back and we're storing the block in the cache compressed if (cacheOnly && cacheCompressed && cacheOnRead) { - LOG.debug("Skipping decompression of block in prefetch"); + LOG.debug("Skipping decompression of block {} in prefetch", cacheKey); // Cache the block if necessary cacheConf.getBlockCache().ifPresent(cache -> { - if (cacheBlock && cacheConf.shouldCacheBlockOnRead(category)) { + if (cacheable && cacheConf.shouldCacheBlockOnRead(category)) { cache.cacheBlock(cacheKey, hfileBlock, cacheConf.isInMemory(), cacheOnly); } }); @@ -1340,7 +1346,7 @@ public HFileBlock readBlock(long dataBlockOffset, long onDiskBlockSize, final bo HFileBlock unpacked = hfileBlock.unpack(hfileContext, fsBlockReader); // Cache the block if necessary cacheConf.getBlockCache().ifPresent(cache -> { - if (cacheBlock && cacheConf.shouldCacheBlockOnRead(category)) { + if (cacheable && cacheConf.shouldCacheBlockOnRead(category)) { // Using the wait on cache during compaction and prefetching. cache.cacheBlock(cacheKey, cacheCompressed ? hfileBlock : unpacked, cacheConf.isInMemory(), cacheOnly); @@ -1667,4 +1673,9 @@ public int getMajorVersion() { public void unbufferStream() { fsBlockReader.unbufferStream(); } + + protected boolean cacheIfCompactionsOff() { + return (!StoreFileInfo.isReference(name) && !HFileLink.isHFileLink(name)) + || !conf.getBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, true); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java index d1cd31867142..cab8004770df 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java @@ -105,7 +105,7 @@ static TransitRegionStateProcedure[] createUnassignProceduresForSplitOrMerge( for (; i < procs.length; i++) { RegionStateNode regionNode = regionNodes.get(i); TransitRegionStateProcedure proc = - TransitRegionStateProcedure.unassign(env, regionNode.getRegionInfo()); + TransitRegionStateProcedure.unassignSplitMerge(env, regionNode.getRegionInfo()); if (regionNode.getProcedure() != null) { throw new HBaseIOException( "The parent region " + regionNode + " is currently in transition, give up"); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index b770d1b16b44..93c964721c46 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -18,6 +18,8 @@ package org.apache.hadoop.hbase.master.assignment; import java.io.IOException; +import java.util.Optional; + import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.exceptions.UnexpectedStateException; @@ -45,14 +47,17 @@ public class CloseRegionProcedure extends RegionRemoteProcedureBase { // wrong(but do not make it wrong intentionally). The client can handle this error. private ServerName assignCandidate; + private Optional evictCache; + public CloseRegionProcedure() { super(); } public CloseRegionProcedure(TransitRegionStateProcedure parent, RegionInfo region, - ServerName targetServer, ServerName assignCandidate) { + ServerName targetServer, ServerName assignCandidate, Optional evictCache) { super(parent, region, targetServer); this.assignCandidate = assignCandidate; + this.evictCache = evictCache; } @Override @@ -62,7 +67,7 @@ public TableOperationType getTableOperationType() { @Override public RemoteOperation newRemoteOperation() { - return new RegionCloseOperation(this, region, getProcId(), assignCandidate); + return new RegionCloseOperation(this, region, getProcId(), assignCandidate, evictCache); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java index df4afaebc1f9..f7464a05a4e6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java @@ -21,6 +21,8 @@ import edu.umd.cs.findbugs.annotations.Nullable; import java.io.IOException; +import java.util.Optional; + import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; @@ -120,6 +122,8 @@ public class TransitRegionStateProcedure private RegionRemoteProcedureBase remoteProc; + private Optional evictCache; + public TransitRegionStateProcedure() { } @@ -145,6 +149,11 @@ private void setInitialAndLastState() { protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, ServerName assignCandidate, boolean forceNewPlan, TransitionType type) { + this(env, hri, assignCandidate, forceNewPlan, type, Optional.empty()); + } + + protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, + ServerName assignCandidate, boolean forceNewPlan, TransitionType type, Optional evictCache) { super(env, hri); this.assignCandidate = assignCandidate; this.forceNewPlan = forceNewPlan; @@ -155,6 +164,8 @@ protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, if (type == TransitionType.REOPEN) { this.assignCandidate = getRegionStateNode(env).getRegionLocation(); } + + this.evictCache = evictCache; } @Override @@ -265,7 +276,7 @@ private void closeRegion(MasterProcedureEnv env, RegionStateNode regionNode) thr // this is the normal case env.getAssignmentManager().regionClosing(regionNode); addChildProcedure(new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), - assignCandidate)); + assignCandidate, evictCache)); setNextState(RegionStateTransitionState.REGION_STATE_TRANSITION_CONFIRM_CLOSED); } else { forceNewPlan = true; @@ -586,6 +597,11 @@ public static TransitRegionStateProcedure unassign(MasterProcedureEnv env, Regio new TransitRegionStateProcedure(env, region, null, false, TransitionType.UNASSIGN)); } + public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, RegionInfo region) { + return setOwner(env, + new TransitRegionStateProcedure(env, region, null, false, TransitionType.UNASSIGN, Optional.of(true))); + } + public static TransitRegionStateProcedure reopen(MasterProcedureEnv env, RegionInfo region) { return setOwner(env, new TransitRegionStateProcedure(env, region, null, false, TransitionType.REOPEN)); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java index a5372240258e..4bcf59c96922 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.lang.Thread.UncaughtExceptionHandler; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.hadoop.hbase.CallQueueTooBigException; @@ -463,11 +464,13 @@ public RegionOpenOperation(RemoteProcedure remoteProcedure, RegionInfo regionInf public static class RegionCloseOperation extends RegionOperation { private final ServerName destinationServer; + private final Optional evictCache; public RegionCloseOperation(RemoteProcedure remoteProcedure, RegionInfo regionInfo, long procId, - ServerName destinationServer) { + ServerName destinationServer, Optional evictCache) { super(remoteProcedure, regionInfo, procId); this.destinationServer = destinationServer; + this.evictCache = evictCache; } public ServerName getDestinationServer() { @@ -475,8 +478,15 @@ public ServerName getDestinationServer() { } public CloseRegionRequest buildCloseRegionRequest(final ServerName serverName) { - return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), - getDestinationServer(), procId); + if (evictCache.isPresent()) { + return ProtobufUtil + .buildCloseRegionRequest(serverName, regionInfo.getRegionName(), getDestinationServer(), + procId, evictCache.get()); + } else { + return ProtobufUtil + .buildCloseRegionRequest(serverName, regionInfo.getRegionName(), getDestinationServer(), + procId); + } } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java index 5b75d3414f1b..4593a7d20020 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java @@ -849,7 +849,7 @@ public boolean isCompactionsEnabled() { public void setCompactionsEnabled(boolean compactionsEnabled) { this.compactionsEnabled = compactionsEnabled; - this.conf.set(HBASE_REGION_SERVER_ENABLE_COMPACTION, String.valueOf(compactionsEnabled)); + this.conf.setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, compactionsEnabled); } /** Returns the longCompactions thread pool executor */ diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 0d8a16ffd420..02a3ba9beb47 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -2962,6 +2962,9 @@ private void closeRegionIgnoreErrors(RegionInfo region, final boolean abort) { *

* If a close was in progress, this new request will be ignored, and an exception thrown. *

+ *

+ * Provides additional flag to indicate if this region blocks should be evicted from the cache. + *

* @param encodedName Region to close * @param abort True if we are aborting * @param destination Where the Region is being moved too... maybe null if unknown. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java index d49c217fe331..8a160c00f8ea 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java @@ -3862,9 +3862,10 @@ private void executeCloseRegionProcedures(CloseRegionRequest request) { ? ProtobufUtil.toServerName(request.getDestinationServer()) : null; long procId = request.getCloseProcId(); + boolean evictCache = request.getEvictCache(); if (server.submitRegionProcedure(procId)) { server.getExecutorService() - .submit(UnassignRegionHandler.create(server, encodedName, procId, false, destination)); + .submit(UnassignRegionHandler.create(server, encodedName, procId, false, destination, evictCache)); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java index 24d47dfbceca..8db7f7d84575 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java @@ -17,6 +17,8 @@ */ package org.apache.hadoop.hbase.regionserver.handler; +import static org.apache.hadoop.hbase.regionserver.CompactSplit.HBASE_REGION_SERVER_ENABLE_COMPACTION; + import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hadoop.hbase.HConstants; @@ -27,6 +29,7 @@ import org.apache.hadoop.hbase.executor.EventType; import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.regionserver.HRegion; +import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.regionserver.RegionServerServices; import org.apache.hadoop.hbase.regionserver.RegionServerServices.PostOpenDeployContext; import org.apache.hadoop.hbase.regionserver.RegionServerServices.RegionStateTransitionContext; @@ -277,6 +280,8 @@ Throwable getException() { /** Returns Instance of HRegion if successful open else null. */ private HRegion openRegion() { HRegion region = null; + boolean compactionEnabled = ((HRegionServer)server).getCompactSplitThread().isCompactionsEnabled(); + this.server.getConfiguration().setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, compactionEnabled); try { // Instantiate the region. This also periodically tickles OPENING // state so master doesn't timeout this region in transition. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java index 32b82264c5ad..29dcc8922239 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java @@ -60,14 +60,22 @@ public class UnassignRegionHandler extends EventHandler { private final RetryCounter retryCounter; + private boolean evictCache; + public UnassignRegionHandler(HRegionServer server, String encodedName, long closeProcId, boolean abort, @Nullable ServerName destination, EventType eventType) { + this(server, encodedName, closeProcId, abort, destination, eventType, false); + } + + public UnassignRegionHandler(HRegionServer server, String encodedName, long closeProcId, + boolean abort, @Nullable ServerName destination, EventType eventType, boolean evictCache) { super(server, eventType); this.encodedName = encodedName; this.closeProcId = closeProcId; this.abort = abort; this.destination = destination; this.retryCounter = HandlerUtil.getRetryCounter(); + this.evictCache = evictCache; } private HRegionServer getServer() { @@ -113,6 +121,12 @@ public void process() throws IOException { // abort the RS... region.getCoprocessorHost().preClose(abort); } + //This should be true only in the case of splits/merges closing the parent regions, as + //there's no point on keep blocks for those region files. As hbase.rs.evictblocksonclose is + //false by default we don't bother overriding it if evictCache is false. + if (evictCache) { + region.getStores().forEach( s -> s.getCacheConfig().setEvictOnClose(true)); + } if (region.close(abort) == null) { // XXX: Is this still possible? The old comment says about split, but now split is done at // master side, so... @@ -144,7 +158,7 @@ protected void handleException(Throwable t) { } public static UnassignRegionHandler create(HRegionServer server, String encodedName, - long closeProcId, boolean abort, @Nullable ServerName destination) { + long closeProcId, boolean abort, @Nullable ServerName destination, boolean evictCache) { // Just try our best to determine whether it is for closing meta. It is not the end of the world // if we put the handler into a wrong executor. Region region = server.getRegion(encodedName); @@ -152,6 +166,6 @@ public static UnassignRegionHandler create(HRegionServer server, String encodedN ? EventType.M_RS_CLOSE_META : EventType.M_RS_CLOSE_REGION; return new UnassignRegionHandler(server, encodedName, closeProcId, abort, destination, - eventType); + eventType, evictCache); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java index 329f5b76f8f0..b2ca4a984b9f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java @@ -20,6 +20,7 @@ import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasName; import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasParentSpanId; import static org.apache.hadoop.hbase.io.hfile.CacheConfig.CACHE_DATA_BLOCKS_COMPRESSED_KEY; +import static org.apache.hadoop.hbase.regionserver.CompactSplit.HBASE_REGION_SERVER_ENABLE_COMPACTION; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.hasItem; @@ -38,6 +39,8 @@ import java.util.concurrent.TimeUnit; import java.util.function.BiConsumer; import java.util.function.BiFunction; +import java.util.function.Consumer; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -46,17 +49,25 @@ import org.apache.hadoop.hbase.HBaseTestingUtil; import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.MatcherPredicate; +import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.trace.StringTraceRenderer; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; import org.apache.hadoop.hbase.fs.HFileSystem; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.io.compress.Compression; +import org.apache.hadoop.hbase.regionserver.BloomType; +import org.apache.hadoop.hbase.regionserver.ConstantSizeRegionSplitPolicy; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.regionserver.HStoreFile; import org.apache.hadoop.hbase.regionserver.StoreFileWriter; import org.apache.hadoop.hbase.testclassification.IOTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.trace.TraceUtil; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; @@ -226,6 +237,53 @@ public void testPrefetchCompressed() throws Exception { } + @Test + public void testPrefetchSkipsRefs() throws Exception { + testPrefetchWhenRefs(true, c -> { + boolean isCached = c!=null; + assertFalse(isCached); + }); + } + + @Test + public void testPrefetchDoesntSkipRefs() throws Exception { + testPrefetchWhenRefs(false, c -> { + boolean isCached = c!=null; + assertTrue(isCached); + }); + } + + private void testPrefetchWhenRefs(boolean compactionEnabled, Consumer test) throws Exception { + cacheConf = new CacheConfig(conf, blockCache); + HFileContext context = new HFileContextBuilder().withBlockSize(DATA_BLOCK_SIZE).build(); + Path tableDir = new Path(TEST_UTIL.getDataTestDir(), "testPrefetchSkipRefs"); + RegionInfo region = RegionInfoBuilder.newBuilder(TableName.valueOf("testPrefetchSkipRefs")).build(); + Path regionDir = new Path(tableDir, region.getEncodedName()); + Pair fileWithSplitPoint = writeStoreFileForSplit(new Path(regionDir, "cf"), context); + Path storeFile = fileWithSplitPoint.getFirst(); + HRegionFileSystem regionFS = HRegionFileSystem.createRegionOnFileSystem(conf, fs, tableDir, region); + HStoreFile file = new HStoreFile(fs, storeFile, conf, cacheConf, BloomType.NONE, true); + Path ref = regionFS.splitStoreFile(region, "cf", file, fileWithSplitPoint.getSecond(), false, + new ConstantSizeRegionSplitPolicy()); + conf.setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, compactionEnabled); + HStoreFile refHsf = new HStoreFile(this.fs, ref, conf, cacheConf, BloomType.NONE, true); + refHsf.initReader(); + HFile.Reader reader = refHsf.getReader().getHFileReader(); + while (!reader.prefetchComplete()) { + // Sleep for a bit + Thread.sleep(1000); + } + long offset = 0; + while (offset < reader.getTrailer().getLoadOnOpenDataOffset()) { + HFileBlock block = reader.readBlock(offset, -1, false, true, false, true, null, null, true); + BlockCacheKey blockCacheKey = new BlockCacheKey(reader.getName(), offset); + if (block.getBlockType() == BlockType.DATA) { + test.accept(blockCache.getBlock(blockCacheKey, true, false, true)); + } + offset += block.getOnDiskSizeWithHeader(); + } + } + private Path writeStoreFile(String fname) throws IOException { HFileContext meta = new HFileContextBuilder().withBlockSize(DATA_BLOCK_SIZE).build(); return writeStoreFile(fname, meta); @@ -250,6 +308,27 @@ private Path writeStoreFile(String fname, HFileContext context) throws IOExcepti return sfw.getPath(); } + private Pair writeStoreFileForSplit(Path storeDir, HFileContext context) throws IOException { + StoreFileWriter sfw = new StoreFileWriter.Builder(conf, cacheConf, fs) + .withOutputDir(storeDir).withFileContext(context).build(); + Random rand = ThreadLocalRandom.current(); + final int rowLen = 32; + byte[] splitPoint = null; + for (int i = 0; i < NUM_KV; ++i) { + byte[] k = RandomKeyValueUtil.randomOrderedKey(rand, i); + byte[] v = RandomKeyValueUtil.randomValue(rand); + int cfLen = rand.nextInt(k.length - rowLen + 1); + KeyValue kv = new KeyValue(k, 0, rowLen, k, rowLen, cfLen, k, rowLen + cfLen, + k.length - rowLen - cfLen, rand.nextLong(), generateKeyType(rand), v, 0, v.length); + sfw.append(kv); + if (i==NUM_KV/2) { + splitPoint = k; + } + } + sfw.close(); + return new Pair(sfw.getPath(), splitPoint); + } + public static KeyValue.Type generateKeyType(Random rand) { if (rand.nextBoolean()) { // Let's make half of KVs puts. From 4ee9264f816a32f727f52d918fadcc8d23401fe4 Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Wed, 9 Nov 2022 11:42:14 +0000 Subject: [PATCH 2/6] checkstyles Change-Id: I7b43530fe2ba44e6b6eb21f6526f7120a8eb235b --- .../hbase/shaded/protobuf/ProtobufUtil.java | 3 ++- .../hadoop/hbase/io/hfile/HFileReaderImpl.java | 3 +-- .../assignment/TransitRegionStateProcedure.java | 9 ++++++--- .../hbase/regionserver/RSRpcServices.java | 3 ++- .../regionserver/handler/OpenRegionHandler.java | 6 ++++-- .../handler/UnassignRegionHandler.java | 2 +- .../hadoop/hbase/io/hfile/TestPrefetch.java | 17 +++++++++++------ 7 files changed, 27 insertions(+), 16 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java index afab0b7050a5..c674e2139339 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java @@ -3010,7 +3010,8 @@ public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte[] regionName, ServerName destinationServer, long closeProcId, boolean evictCache) { - CloseRegionRequest.Builder builder = getBuilder(server, regionName, destinationServer, closeProcId); + CloseRegionRequest.Builder builder = + getBuilder(server, regionName, destinationServer, closeProcId); builder.setEvictCache(evictCache); return builder.build(); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java index fb33f6df0323..e181a61de347 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java @@ -17,10 +17,10 @@ */ package org.apache.hadoop.hbase.io.hfile; +import static org.apache.hadoop.hbase.regionserver.CompactSplit.HBASE_REGION_SERVER_ENABLE_COMPACTION; import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.BLOCK_CACHE_KEY_KEY; import io.opentelemetry.api.common.Attributes; -import static org.apache.hadoop.hbase.regionserver.CompactSplit.HBASE_REGION_SERVER_ENABLE_COMPACTION; import io.opentelemetry.api.trace.Span; import java.io.DataInput; import java.io.IOException; @@ -49,7 +49,6 @@ import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.regionserver.KeyValueScanner; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; -import org.apache.hadoop.hbase.trace.TraceUtil; import org.apache.hadoop.hbase.util.ByteBufferUtils; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.IdLock; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java index f7464a05a4e6..c70181ea1ab1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java @@ -153,7 +153,8 @@ protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, } protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, - ServerName assignCandidate, boolean forceNewPlan, TransitionType type, Optional evictCache) { + ServerName assignCandidate, boolean forceNewPlan, TransitionType type, + Optional evictCache) { super(env, hri); this.assignCandidate = assignCandidate; this.forceNewPlan = forceNewPlan; @@ -597,9 +598,11 @@ public static TransitRegionStateProcedure unassign(MasterProcedureEnv env, Regio new TransitRegionStateProcedure(env, region, null, false, TransitionType.UNASSIGN)); } - public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, RegionInfo region) { + public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, + RegionInfo region) { return setOwner(env, - new TransitRegionStateProcedure(env, region, null, false, TransitionType.UNASSIGN, Optional.of(true))); + new TransitRegionStateProcedure(env, region, null, false, + TransitionType.UNASSIGN, Optional.of(true))); } public static TransitRegionStateProcedure reopen(MasterProcedureEnv env, RegionInfo region) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java index 8a160c00f8ea..238e06a73c7f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java @@ -3865,7 +3865,8 @@ private void executeCloseRegionProcedures(CloseRegionRequest request) { boolean evictCache = request.getEvictCache(); if (server.submitRegionProcedure(procId)) { server.getExecutorService() - .submit(UnassignRegionHandler.create(server, encodedName, procId, false, destination, evictCache)); + .submit(UnassignRegionHandler.create(server, encodedName, procId, false, + destination, evictCache)); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java index 8db7f7d84575..6b384587cbd6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java @@ -280,8 +280,10 @@ Throwable getException() { /** Returns Instance of HRegion if successful open else null. */ private HRegion openRegion() { HRegion region = null; - boolean compactionEnabled = ((HRegionServer)server).getCompactSplitThread().isCompactionsEnabled(); - this.server.getConfiguration().setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, compactionEnabled); + boolean compactionEnabled = + ((HRegionServer)server).getCompactSplitThread().isCompactionsEnabled(); + this.server.getConfiguration().setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, + compactionEnabled); try { // Instantiate the region. This also periodically tickles OPENING // state so master doesn't timeout this region in transition. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java index 29dcc8922239..bf890bc575f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java @@ -125,7 +125,7 @@ public void process() throws IOException { //there's no point on keep blocks for those region files. As hbase.rs.evictblocksonclose is //false by default we don't bother overriding it if evictCache is false. if (evictCache) { - region.getStores().forEach( s -> s.getCacheConfig().setEvictOnClose(true)); + region.getStores().forEach(s -> s.getCacheConfig().setEvictOnClose(true)); } if (region.close(abort) == null) { // XXX: Is this still possible? The old comment says about split, but now split is done at diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java index b2ca4a984b9f..3b4e6be08441 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java @@ -52,9 +52,9 @@ import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; -import org.apache.hadoop.hbase.client.trace.StringTraceRenderer; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionInfoBuilder; +import org.apache.hadoop.hbase.client.trace.StringTraceRenderer; import org.apache.hadoop.hbase.fs.HFileSystem; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.io.compress.Compression; @@ -253,15 +253,19 @@ public void testPrefetchDoesntSkipRefs() throws Exception { }); } - private void testPrefetchWhenRefs(boolean compactionEnabled, Consumer test) throws Exception { + private void testPrefetchWhenRefs(boolean compactionEnabled, Consumer test) + throws Exception { cacheConf = new CacheConfig(conf, blockCache); HFileContext context = new HFileContextBuilder().withBlockSize(DATA_BLOCK_SIZE).build(); Path tableDir = new Path(TEST_UTIL.getDataTestDir(), "testPrefetchSkipRefs"); - RegionInfo region = RegionInfoBuilder.newBuilder(TableName.valueOf("testPrefetchSkipRefs")).build(); + RegionInfo region = + RegionInfoBuilder.newBuilder(TableName.valueOf("testPrefetchSkipRefs")).build(); Path regionDir = new Path(tableDir, region.getEncodedName()); - Pair fileWithSplitPoint = writeStoreFileForSplit(new Path(regionDir, "cf"), context); + Pair fileWithSplitPoint = + writeStoreFileForSplit(new Path(regionDir, "cf"), context); Path storeFile = fileWithSplitPoint.getFirst(); - HRegionFileSystem regionFS = HRegionFileSystem.createRegionOnFileSystem(conf, fs, tableDir, region); + HRegionFileSystem regionFS = + HRegionFileSystem.createRegionOnFileSystem(conf, fs, tableDir, region); HStoreFile file = new HStoreFile(fs, storeFile, conf, cacheConf, BloomType.NONE, true); Path ref = regionFS.splitStoreFile(region, "cf", file, fileWithSplitPoint.getSecond(), false, new ConstantSizeRegionSplitPolicy()); @@ -308,7 +312,8 @@ private Path writeStoreFile(String fname, HFileContext context) throws IOExcepti return sfw.getPath(); } - private Pair writeStoreFileForSplit(Path storeDir, HFileContext context) throws IOException { + private Pair writeStoreFileForSplit(Path storeDir, HFileContext context) + throws IOException { StoreFileWriter sfw = new StoreFileWriter.Builder(conf, cacheConf, fs) .withOutputDir(storeDir).withFileContext(context).build(); Random rand = ThreadLocalRandom.current(); From 525137f0600cb376a69cfc6aba06c0834e39bd66 Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Wed, 9 Nov 2022 11:44:56 +0000 Subject: [PATCH 3/6] spotless Change-Id: Ia6bcfcb9445e4a448073474d12bda549cf5c6edd --- .../hadoop/hbase/shaded/protobuf/ProtobufUtil.java | 2 +- .../hbase/master/assignment/CloseRegionProcedure.java | 1 - .../assignment/TransitRegionStateProcedure.java | 6 ++---- .../hbase/master/procedure/RSProcedureDispatcher.java | 10 ++++------ .../hadoop/hbase/regionserver/RSRpcServices.java | 5 ++--- .../hbase/regionserver/handler/OpenRegionHandler.java | 2 +- .../regionserver/handler/UnassignRegionHandler.java | 6 +++--- .../apache/hadoop/hbase/io/hfile/TestPrefetch.java | 11 +++++------ 8 files changed, 18 insertions(+), 25 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java index c674e2139339..ca38a91e74a6 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java @@ -3017,7 +3017,7 @@ public static CloseRegionRequest buildCloseRegionRequest(ServerName server, byte } public static CloseRegionRequest.Builder getBuilder(ServerName server, byte[] regionName, - ServerName destinationServer, long closeProcId){ + ServerName destinationServer, long closeProcId) { CloseRegionRequest.Builder builder = CloseRegionRequest.newBuilder(); RegionSpecifier region = RequestConverter.buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index 93c964721c46..90c5184dc5c6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -19,7 +19,6 @@ import java.io.IOException; import java.util.Optional; - import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.exceptions.UnexpectedStateException; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java index c70181ea1ab1..61fd84282382 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java @@ -22,7 +22,6 @@ import edu.umd.cs.findbugs.annotations.Nullable; import java.io.IOException; import java.util.Optional; - import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; @@ -600,9 +599,8 @@ public static TransitRegionStateProcedure unassign(MasterProcedureEnv env, Regio public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, RegionInfo region) { - return setOwner(env, - new TransitRegionStateProcedure(env, region, null, false, - TransitionType.UNASSIGN, Optional.of(true))); + return setOwner(env, new TransitRegionStateProcedure(env, region, null, false, + TransitionType.UNASSIGN, Optional.of(true))); } public static TransitRegionStateProcedure reopen(MasterProcedureEnv env, RegionInfo region) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java index 4bcf59c96922..a94c3ddedb97 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java @@ -479,13 +479,11 @@ public ServerName getDestinationServer() { public CloseRegionRequest buildCloseRegionRequest(final ServerName serverName) { if (evictCache.isPresent()) { - return ProtobufUtil - .buildCloseRegionRequest(serverName, regionInfo.getRegionName(), getDestinationServer(), - procId, evictCache.get()); + return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), + getDestinationServer(), procId, evictCache.get()); } else { - return ProtobufUtil - .buildCloseRegionRequest(serverName, regionInfo.getRegionName(), getDestinationServer(), - procId); + return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), + getDestinationServer(), procId); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java index 238e06a73c7f..4201a6d65aef 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java @@ -3864,9 +3864,8 @@ private void executeCloseRegionProcedures(CloseRegionRequest request) { long procId = request.getCloseProcId(); boolean evictCache = request.getEvictCache(); if (server.submitRegionProcedure(procId)) { - server.getExecutorService() - .submit(UnassignRegionHandler.create(server, encodedName, procId, false, - destination, evictCache)); + server.getExecutorService().submit( + UnassignRegionHandler.create(server, encodedName, procId, false, destination, evictCache)); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java index 6b384587cbd6..898121602a4e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java @@ -281,7 +281,7 @@ Throwable getException() { private HRegion openRegion() { HRegion region = null; boolean compactionEnabled = - ((HRegionServer)server).getCompactSplitThread().isCompactionsEnabled(); + ((HRegionServer) server).getCompactSplitThread().isCompactionsEnabled(); this.server.getConfiguration().setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION, compactionEnabled); try { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java index bf890bc575f3..9d218fa45c4b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java @@ -121,9 +121,9 @@ public void process() throws IOException { // abort the RS... region.getCoprocessorHost().preClose(abort); } - //This should be true only in the case of splits/merges closing the parent regions, as - //there's no point on keep blocks for those region files. As hbase.rs.evictblocksonclose is - //false by default we don't bother overriding it if evictCache is false. + // This should be true only in the case of splits/merges closing the parent regions, as + // there's no point on keep blocks for those region files. As hbase.rs.evictblocksonclose is + // false by default we don't bother overriding it if evictCache is false. if (evictCache) { region.getStores().forEach(s -> s.getCacheConfig().setEvictOnClose(true)); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java index 3b4e6be08441..e7aad2650d42 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java @@ -40,7 +40,6 @@ import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; - import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -240,7 +239,7 @@ public void testPrefetchCompressed() throws Exception { @Test public void testPrefetchSkipsRefs() throws Exception { testPrefetchWhenRefs(true, c -> { - boolean isCached = c!=null; + boolean isCached = c != null; assertFalse(isCached); }); } @@ -248,7 +247,7 @@ public void testPrefetchSkipsRefs() throws Exception { @Test public void testPrefetchDoesntSkipRefs() throws Exception { testPrefetchWhenRefs(false, c -> { - boolean isCached = c!=null; + boolean isCached = c != null; assertTrue(isCached); }); } @@ -314,8 +313,8 @@ private Path writeStoreFile(String fname, HFileContext context) throws IOExcepti private Pair writeStoreFileForSplit(Path storeDir, HFileContext context) throws IOException { - StoreFileWriter sfw = new StoreFileWriter.Builder(conf, cacheConf, fs) - .withOutputDir(storeDir).withFileContext(context).build(); + StoreFileWriter sfw = new StoreFileWriter.Builder(conf, cacheConf, fs).withOutputDir(storeDir) + .withFileContext(context).build(); Random rand = ThreadLocalRandom.current(); final int rowLen = 32; byte[] splitPoint = null; @@ -326,7 +325,7 @@ private Pair writeStoreFileForSplit(Path storeDir, HFileContext co KeyValue kv = new KeyValue(k, 0, rowLen, k, rowLen, cfLen, k, rowLen + cfLen, k.length - rowLen - cfLen, rand.nextLong(), generateKeyType(rand), v, 0, v.length); sfw.append(kv); - if (i==NUM_KV/2) { + if (i == NUM_KV / 2) { splitPoint = k; } } From 0367419f796feacaa68dd7ccd99f8d8e1dd189d7 Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Sat, 26 Nov 2022 15:47:44 +0000 Subject: [PATCH 4/6] removing optionals; adding extra field serialization on procs; --- .../assignment/CloseRegionProcedure.java | 10 +++--- .../TransitRegionStateProcedure.java | 36 ++++++++++++------- .../procedure/RSProcedureDispatcher.java | 14 +++----- 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index 90c5184dc5c6..1d827f426282 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -18,7 +18,6 @@ package org.apache.hadoop.hbase.master.assignment; import java.io.IOException; -import java.util.Optional; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.exceptions.UnexpectedStateException; @@ -46,14 +45,14 @@ public class CloseRegionProcedure extends RegionRemoteProcedureBase { // wrong(but do not make it wrong intentionally). The client can handle this error. private ServerName assignCandidate; - private Optional evictCache; + private boolean evictCache; public CloseRegionProcedure() { super(); } public CloseRegionProcedure(TransitRegionStateProcedure parent, RegionInfo region, - ServerName targetServer, ServerName assignCandidate, Optional evictCache) { + ServerName targetServer, ServerName assignCandidate, boolean evictCache) { super(parent, region, targetServer); this.assignCandidate = assignCandidate; this.evictCache = evictCache; @@ -66,7 +65,8 @@ public TableOperationType getTableOperationType() { @Override public RemoteOperation newRemoteOperation() { - return new RegionCloseOperation(this, region, getProcId(), assignCandidate, evictCache); + return new RegionCloseOperation(this, region, getProcId(), + assignCandidate, evictCache); } @Override @@ -76,6 +76,7 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO if (assignCandidate != null) { builder.setAssignCandidate(ProtobufUtil.toServerName(assignCandidate)); } + builder.setEvictCache(evictCache); serializer.serialize(builder.build()); } @@ -87,6 +88,7 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws if (data.hasAssignCandidate()) { assignCandidate = ProtobufUtil.toServerName(data.getAssignCandidate()); } + evictCache = data.getEvictCache(); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java index 61fd84282382..ebed6d2a951a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java @@ -17,11 +17,12 @@ */ package org.apache.hadoop.hbase.master.assignment; +import static org.apache.hadoop.hbase.io.hfile.CacheConfig.DEFAULT_EVICT_ON_CLOSE; +import static org.apache.hadoop.hbase.io.hfile.CacheConfig.EVICT_BLOCKS_ON_CLOSE_KEY; import static org.apache.hadoop.hbase.master.LoadBalancer.BOGUS_SERVER_NAME; import edu.umd.cs.findbugs.annotations.Nullable; import java.io.IOException; -import java.util.Optional; import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; @@ -121,7 +122,9 @@ public class TransitRegionStateProcedure private RegionRemoteProcedureBase remoteProc; - private Optional evictCache; + private boolean evictCache; + + private boolean isSplit; public TransitRegionStateProcedure() { } @@ -148,12 +151,6 @@ private void setInitialAndLastState() { protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, ServerName assignCandidate, boolean forceNewPlan, TransitionType type) { - this(env, hri, assignCandidate, forceNewPlan, type, Optional.empty()); - } - - protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, - ServerName assignCandidate, boolean forceNewPlan, TransitionType type, - Optional evictCache) { super(env, hri); this.assignCandidate = assignCandidate; this.forceNewPlan = forceNewPlan; @@ -164,8 +161,14 @@ protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, if (type == TransitionType.REOPEN) { this.assignCandidate = getRegionStateNode(env).getRegionLocation(); } + evictCache = env.getMasterConfiguration().getBoolean(EVICT_BLOCKS_ON_CLOSE_KEY, DEFAULT_EVICT_ON_CLOSE); + } - this.evictCache = evictCache; + protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, + ServerName assignCandidate, boolean forceNewPlan, TransitionType type, + boolean isSplit) { + this(env,hri, assignCandidate, forceNewPlan, type); + this.isSplit = isSplit; } @Override @@ -275,8 +278,12 @@ private void closeRegion(MasterProcedureEnv env, RegionStateNode regionNode) thr if (regionNode.isInState(State.OPEN, State.CLOSING, State.MERGING, State.SPLITTING)) { // this is the normal case env.getAssignmentManager().regionClosing(regionNode); - addChildProcedure(new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), - assignCandidate, evictCache)); + CloseRegionProcedure closeProc = isSplit ? + new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), + assignCandidate, true) : + new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), + assignCandidate, evictCache); + addChildProcedure(closeProc); setNextState(RegionStateTransitionState.REGION_STATE_TRANSITION_CONFIRM_CLOSED); } else { forceNewPlan = true; @@ -516,7 +523,8 @@ private static RegionTransitionType convert(TransitionType type) { protected void serializeStateData(ProcedureStateSerializer serializer) throws IOException { super.serializeStateData(serializer); RegionStateTransitionStateData.Builder builder = RegionStateTransitionStateData.newBuilder() - .setType(convert(type)).setForceNewPlan(forceNewPlan); + .setType(convert(type)).setForceNewPlan(forceNewPlan).setEvictCache(evictCache) + .setIsSplit(isSplit); if (assignCandidate != null) { builder.setAssignCandidate(ProtobufUtil.toServerName(assignCandidate)); } @@ -534,6 +542,8 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws if (data.hasAssignCandidate()) { assignCandidate = ProtobufUtil.toServerName(data.getAssignCandidate()); } + evictCache = data.getEvictCache(); + isSplit = data.getIsSplit(); } @Override @@ -600,7 +610,7 @@ public static TransitRegionStateProcedure unassign(MasterProcedureEnv env, Regio public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, RegionInfo region) { return setOwner(env, new TransitRegionStateProcedure(env, region, null, false, - TransitionType.UNASSIGN, Optional.of(true))); + TransitionType.UNASSIGN, true)); } public static TransitRegionStateProcedure reopen(MasterProcedureEnv env, RegionInfo region) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java index a94c3ddedb97..b47aec09571a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java @@ -464,10 +464,10 @@ public RegionOpenOperation(RemoteProcedure remoteProcedure, RegionInfo regionInf public static class RegionCloseOperation extends RegionOperation { private final ServerName destinationServer; - private final Optional evictCache; + private boolean evictCache; public RegionCloseOperation(RemoteProcedure remoteProcedure, RegionInfo regionInfo, long procId, - ServerName destinationServer, Optional evictCache) { + ServerName destinationServer, boolean evictCache) { super(remoteProcedure, regionInfo, procId); this.destinationServer = destinationServer; this.evictCache = evictCache; @@ -478,13 +478,9 @@ public ServerName getDestinationServer() { } public CloseRegionRequest buildCloseRegionRequest(final ServerName serverName) { - if (evictCache.isPresent()) { - return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), - getDestinationServer(), procId, evictCache.get()); - } else { - return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), - getDestinationServer(), procId); - } + return ProtobufUtil.buildCloseRegionRequest(serverName, regionInfo.getRegionName(), + getDestinationServer(), procId, evictCache); + } } } From e6905574a7d4193f004bf88c809d111d9b982a70 Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Sat, 26 Nov 2022 15:49:00 +0000 Subject: [PATCH 5/6] master proto changes --- .../src/main/protobuf/server/master/MasterProcedure.proto | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto index 35125a5a94e6..59bb031589af 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -604,6 +604,8 @@ message RegionStateTransitionStateData { required RegionTransitionType type = 1; optional ServerName assign_candidate = 2; required bool force_new_plan = 3; + optional bool is_split = 4 [default = false]; + optional bool evict_cache = 5 [default = false]; } enum RegionRemoteProcedureBaseState { @@ -628,6 +630,7 @@ message OpenRegionProcedureStateData { message CloseRegionProcedureStateData { optional ServerName assign_candidate = 1; + optional bool evict_cache = 2 [default = false]; } enum SwitchRpcThrottleState { From 32114b457879522a1255ce90675266661482bf20 Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Mon, 28 Nov 2022 12:28:13 +0000 Subject: [PATCH 6/6] spotless fixes --- .../assignment/CloseRegionProcedure.java | 3 +-- .../TransitRegionStateProcedure.java | 26 +++++++++---------- .../procedure/RSProcedureDispatcher.java | 1 - 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index 1d827f426282..f51af7ac0d58 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -65,8 +65,7 @@ public TableOperationType getTableOperationType() { @Override public RemoteOperation newRemoteOperation() { - return new RegionCloseOperation(this, region, getProcId(), - assignCandidate, evictCache); + return new RegionCloseOperation(this, region, getProcId(), assignCandidate, evictCache); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java index ebed6d2a951a..72ac2d3827fd 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java @@ -161,13 +161,13 @@ protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, if (type == TransitionType.REOPEN) { this.assignCandidate = getRegionStateNode(env).getRegionLocation(); } - evictCache = env.getMasterConfiguration().getBoolean(EVICT_BLOCKS_ON_CLOSE_KEY, DEFAULT_EVICT_ON_CLOSE); + evictCache = + env.getMasterConfiguration().getBoolean(EVICT_BLOCKS_ON_CLOSE_KEY, DEFAULT_EVICT_ON_CLOSE); } protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri, - ServerName assignCandidate, boolean forceNewPlan, TransitionType type, - boolean isSplit) { - this(env,hri, assignCandidate, forceNewPlan, type); + ServerName assignCandidate, boolean forceNewPlan, TransitionType type, boolean isSplit) { + this(env, hri, assignCandidate, forceNewPlan, type); this.isSplit = isSplit; } @@ -278,10 +278,10 @@ private void closeRegion(MasterProcedureEnv env, RegionStateNode regionNode) thr if (regionNode.isInState(State.OPEN, State.CLOSING, State.MERGING, State.SPLITTING)) { // this is the normal case env.getAssignmentManager().regionClosing(regionNode); - CloseRegionProcedure closeProc = isSplit ? - new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), - assignCandidate, true) : - new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), + CloseRegionProcedure closeProc = isSplit + ? new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), + assignCandidate, true) + : new CloseRegionProcedure(this, getRegion(), regionNode.getRegionLocation(), assignCandidate, evictCache); addChildProcedure(closeProc); setNextState(RegionStateTransitionState.REGION_STATE_TRANSITION_CONFIRM_CLOSED); @@ -522,9 +522,9 @@ private static RegionTransitionType convert(TransitionType type) { @Override protected void serializeStateData(ProcedureStateSerializer serializer) throws IOException { super.serializeStateData(serializer); - RegionStateTransitionStateData.Builder builder = RegionStateTransitionStateData.newBuilder() - .setType(convert(type)).setForceNewPlan(forceNewPlan).setEvictCache(evictCache) - .setIsSplit(isSplit); + RegionStateTransitionStateData.Builder builder = + RegionStateTransitionStateData.newBuilder().setType(convert(type)) + .setForceNewPlan(forceNewPlan).setEvictCache(evictCache).setIsSplit(isSplit); if (assignCandidate != null) { builder.setAssignCandidate(ProtobufUtil.toServerName(assignCandidate)); } @@ -609,8 +609,8 @@ public static TransitRegionStateProcedure unassign(MasterProcedureEnv env, Regio public static TransitRegionStateProcedure unassignSplitMerge(MasterProcedureEnv env, RegionInfo region) { - return setOwner(env, new TransitRegionStateProcedure(env, region, null, false, - TransitionType.UNASSIGN, true)); + return setOwner(env, + new TransitRegionStateProcedure(env, region, null, false, TransitionType.UNASSIGN, true)); } public static TransitRegionStateProcedure reopen(MasterProcedureEnv env, RegionInfo region) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java index b47aec09571a..af22fba27290 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.lang.Thread.UncaughtExceptionHandler; import java.util.List; -import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.hadoop.hbase.CallQueueTooBigException;