From 71d43fb07c66234ef140e3bbcfb718969ae2a804 Mon Sep 17 00:00:00 2001 From: ukumawat Date: Thu, 12 Dec 2024 17:39:10 +0530 Subject: [PATCH 1/6] HBASE-28951 rename the wal before retrying the wal-split with another worker --- .../hadoop/hbase/master/SplitWALManager.java | 10 ++++++ .../master/procedure/SplitWALProcedure.java | 31 +++++++++++++------ .../hbase/wal/AbstractFSWALProvider.java | 2 ++ 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index 18dfc7d493bf..8c3ae31b42cf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -21,6 +21,7 @@ import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER; import static org.apache.hadoop.hbase.master.MasterWalManager.META_FILTER; import static org.apache.hadoop.hbase.master.MasterWalManager.NON_META_FILTER; +import static org.apache.hadoop.hbase.wal.AbstractFSWALProvider.RETRYING_EXT; import java.io.IOException; import java.util.Arrays; @@ -184,4 +185,13 @@ public void releaseSplitWALWorker(ServerName worker, MasterProcedureScheduler sc public void addUsedSplitWALWorker(ServerName worker) { splitWorkerAssigner.addUsedWorker(worker); } + + public String renameWALForRetry(String walPath) throws IOException { + Path walCurrentPath = new Path(rootDir, walPath); + Path walNewPath = walPath.endsWith(RETRYING_EXT) + ? new Path(rootDir, walPath.substring(0, walPath.length() - RETRYING_EXT.length())) + : walCurrentPath.suffix(RETRYING_EXT); + fs.rename(walCurrentPath, walNewPath); + return walNewPath.toString(); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java index 699834f9c1d7..4bc8bbcb7268 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java @@ -79,20 +79,22 @@ protected Flow executeFromState(MasterProcedureEnv env, MasterProcedureProtos.Sp try { finished = splitWALManager.isSplitWALFinished(walPath); } catch (IOException ioe) { - if (retryCounter == null) { - retryCounter = ProcedureUtil.createRetryCounter(env.getMasterConfiguration()); - } - long backoff = retryCounter.getBackoffTimeAndIncrementAttempts(); - LOG.warn("Failed to check whether splitting wal {} success, wait {} seconds to retry", - walPath, backoff / 1000, ioe); - setTimeout(Math.toIntExact(backoff)); - setState(ProcedureProtos.ProcedureState.WAITING_TIMEOUT); - skipPersistence(); + LOG.warn("Failed to check whether splitting wal {} success", walPath); + setTimeoutForSuspend(env, ioe.getMessage()); throw new ProcedureSuspendedException(); } splitWALManager.releaseSplitWALWorker(worker, env.getProcedureScheduler()); if (!finished) { LOG.warn("Failed to split wal {} by server {}, retry...", walPath, worker); + // HBASE-28951 in case of a failure because unresponsive rs, rs might still be processing + // the split wal + try { + this.walPath = splitWALManager.renameWALForRetry(this.walPath); + } catch (IOException ioe) { + LOG.warn("Failed to rename the splitting wal {}", walPath); + setTimeoutForSuspend(env, ioe.getMessage()); + throw new ProcedureSuspendedException(); + } setNextState(MasterProcedureProtos.SplitWALState.ACQUIRE_SPLIT_WAL_WORKER); return Flow.HAS_MORE_STATE; } @@ -103,6 +105,17 @@ protected Flow executeFromState(MasterProcedureEnv env, MasterProcedureProtos.Sp } } + private void setTimeoutForSuspend(MasterProcedureEnv env, String reason) { + if (retryCounter == null) { + retryCounter = ProcedureUtil.createRetryCounter(env.getMasterConfiguration()); + } + long backoff = retryCounter.getBackoffTimeAndIncrementAttempts(); + LOG.warn("{} can not run currently because {}, wait {} ms to retry", this, reason, backoff); + setTimeout(Math.toIntExact(backoff)); + setState(ProcedureProtos.ProcedureState.WAITING_TIMEOUT); + skipPersistence(); + } + @Override protected void rollbackState(MasterProcedureEnv env, MasterProcedureProtos.SplitWALState splitOneWalState) throws IOException, InterruptedException { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java index 13d2886182e4..d705d9be9915 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java @@ -237,6 +237,8 @@ static void requestLogRoll(final WAL wal) { /** File Extension used while splitting an WAL into regions (HBASE-2312) */ public static final String SPLITTING_EXT = "-splitting"; + public static final String RETRYING_EXT = ".retrying"; + /** * Pattern used to validate a WAL file name see {@link #validateWALFilename(String)} for * description. From 56423a79a202ff00a6909128e322749aa09fab67 Mon Sep 17 00:00:00 2001 From: ukumawat Date: Fri, 13 Dec 2024 00:20:58 +0530 Subject: [PATCH 2/6] HBASE-28951 add a counter for worker change to keep different name each time --- .../server/master/MasterProcedure.proto | 1 + .../hadoop/hbase/master/SplitWALManager.java | 20 ++++++---- .../master/procedure/SplitWALProcedure.java | 12 +++++- .../hbase/master/TestSplitWALManager.java | 40 +++++++++++++++++++ 4 files changed, 64 insertions(+), 9 deletions(-) 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 81d16b2861ca..49d01da9a9f8 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -709,6 +709,7 @@ message SplitWALData{ required string wal_path = 1; required ServerName crashed_server=2; optional ServerName worker = 3; + optional uint32 workerChangeCount =4; } message SplitWALRemoteData{ diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index 8c3ae31b42cf..245b34d55172 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -186,12 +186,18 @@ public void addUsedSplitWALWorker(ServerName worker) { splitWorkerAssigner.addUsedWorker(worker); } - public String renameWALForRetry(String walPath) throws IOException { - Path walCurrentPath = new Path(rootDir, walPath); - Path walNewPath = walPath.endsWith(RETRYING_EXT) - ? new Path(rootDir, walPath.substring(0, walPath.length() - RETRYING_EXT.length())) - : walCurrentPath.suffix(RETRYING_EXT); - fs.rename(walCurrentPath, walNewPath); - return walNewPath.toString(); + public String renameWALForRetry(String walPath, Integer workerChangeCount) throws IOException { + String originalWALPath; + if (workerChangeCount == 0) { + originalWALPath = walPath; + } else { + originalWALPath = walPath.substring(0, walPath.length() - RETRYING_EXT.length() - 3); + } + String walNewName = + originalWALPath + RETRYING_EXT + String.format("%03d", (workerChangeCount + 1) % 1000); + if (!fs.rename(new Path(rootDir, walPath), new Path(rootDir, walNewName))) { + throw new IOException("Failed to rename wal " + walPath + " to " + walNewName); + } + return walNewName; } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java index 4bc8bbcb7268..28fefe8d9e6e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java @@ -51,6 +51,7 @@ public class SplitWALProcedure private ServerName worker; private ServerName crashedServer; private RetryCounter retryCounter; + private Integer workerChangeCount = 0; public SplitWALProcedure() { } @@ -89,7 +90,8 @@ protected Flow executeFromState(MasterProcedureEnv env, MasterProcedureProtos.Sp // HBASE-28951 in case of a failure because unresponsive rs, rs might still be processing // the split wal try { - this.walPath = splitWALManager.renameWALForRetry(this.walPath); + this.walPath = splitWALManager.renameWALForRetry(this.walPath, workerChangeCount); + workerChangeCount++; } catch (IOException ioe) { LOG.warn("Failed to rename the splitting wal {}", walPath); setTimeoutForSuspend(env, ioe.getMessage()); @@ -145,7 +147,8 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO super.serializeStateData(serializer); MasterProcedureProtos.SplitWALData.Builder builder = MasterProcedureProtos.SplitWALData.newBuilder(); - builder.setWalPath(walPath).setCrashedServer(ProtobufUtil.toServerName(crashedServer)); + builder.setWalPath(walPath).setCrashedServer(ProtobufUtil.toServerName(crashedServer)) + .setWorkerChangeCount(workerChangeCount); if (worker != null) { builder.setWorker(ProtobufUtil.toServerName(worker)); } @@ -159,6 +162,7 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws serializer.deserialize(MasterProcedureProtos.SplitWALData.class); walPath = data.getWalPath(); crashedServer = ProtobufUtil.toServerName(data.getCrashedServer()); + workerChangeCount = data.getWorkerChangeCount(); if (data.hasWorker()) { worker = ProtobufUtil.toServerName(data.getWorker()); } @@ -179,6 +183,10 @@ public ServerName getWorker() { return worker; } + public Integer getWorkerChangeCount() { + return workerChangeCount; + } + @Override public ServerName getServerName() { return this.crashedServer; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java index ea92f7922794..f053461fb6e9 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java @@ -19,10 +19,13 @@ import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK; import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER; +import static org.apache.hadoop.hbase.master.MasterWalManager.NON_META_FILTER; import static org.apache.hadoop.hbase.master.procedure.ServerProcedureInterface.ServerOperationType.SPLIT_WAL; +import static org.apache.hadoop.hbase.wal.AbstractFSWALProvider.RETRYING_EXT; import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.concurrent.CountDownLatch; import org.apache.hadoop.fs.FileStatus; @@ -42,6 +45,7 @@ import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; import org.apache.hadoop.hbase.procedure2.ProcedureYieldException; import org.apache.hadoop.hbase.procedure2.StateMachineProcedure; +import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.util.Bytes; @@ -94,6 +98,42 @@ public void teardown() throws Exception { TEST_UTIL.shutdownMiniCluster(); } + @Test + public void testRenameWALForRetry() throws Exception { + HRegionServer regionServer = TEST_UTIL.getHBaseCluster().getRegionServer(0); + List logDirs = + master.getMasterWalManager().getLogDirs(Collections.singleton(regionServer.getServerName())); + List list = + SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); + Path testWal = list.get(0).getPath(); + int workerChangeCount = 0; + + String newWALPath = splitWALManager.renameWALForRetry(testWal.toString(), workerChangeCount); + Assert.assertEquals(newWALPath, + testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); + workerChangeCount = workerChangeCount + 1; + + newWALPath = splitWALManager.renameWALForRetry(newWALPath, workerChangeCount); + Assert.assertEquals(newWALPath, + testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); + + List fileStatusesA = + SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); + + Path walFromFileSystem = null; + for (FileStatus wal : fileStatusesA) { + if ( + wal.getPath().toString() + .endsWith(RETRYING_EXT + String.format("%03d", workerChangeCount + 1)) + ) { + walFromFileSystem = wal.getPath(); + break; + } + } + Assert.assertNotNull(walFromFileSystem); + Assert.assertEquals(walFromFileSystem.toString(), newWALPath); + } + @Test public void testAcquireAndRelease() throws Exception { List testProcedures = new ArrayList<>(); From cb375de2c514a4e354969cefcb928a498457676c Mon Sep 17 00:00:00 2001 From: ukumawat Date: Mon, 16 Dec 2024 22:48:30 +0530 Subject: [PATCH 3/6] HBASE-28951 handle first retry of splitWal for a wal with .retrying suffix --- .../hadoop/hbase/master/SplitWALManager.java | 12 ++++-- .../master/procedure/SplitWALProcedure.java | 6 +-- .../hbase/wal/AbstractFSWALProvider.java | 1 + .../hbase/master/TestSplitWALManager.java | 41 ++++++++++++++++++- 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index 245b34d55172..c47b66e4dc04 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -188,10 +188,16 @@ public void addUsedSplitWALWorker(ServerName worker) { public String renameWALForRetry(String walPath, Integer workerChangeCount) throws IOException { String originalWALPath; - if (workerChangeCount == 0) { - originalWALPath = walPath; - } else { + // If the WAL split is retried with another worker, ".retrying-xxx" is appended to the walPath. + // In the case where a SplitWalProcedure is bypassed or rolled back after being retried with + // another worker, the walPath will still have ".retrying-xxx" appended. During recovery, a new + // SplitWalProcedure (by different SCP) will be created, which will have a workerChangeCount of + // 0 but will still have ".retrying-xxx" appended to the walPath. This is why we cannot use the + // condition workerChangeCount != 0. + if (walPath.substring(walPath.length() - RETRYING_EXT.length() - 3).startsWith(RETRYING_EXT)) { originalWALPath = walPath.substring(0, walPath.length() - RETRYING_EXT.length() - 3); + } else { + originalWALPath = walPath; } String walNewName = originalWALPath + RETRYING_EXT + String.format("%03d", (workerChangeCount + 1) % 1000); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java index 28fefe8d9e6e..eb96071785db 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java @@ -51,7 +51,7 @@ public class SplitWALProcedure private ServerName worker; private ServerName crashedServer; private RetryCounter retryCounter; - private Integer workerChangeCount = 0; + private int workerChangeCount = 0; public SplitWALProcedure() { } @@ -183,10 +183,6 @@ public ServerName getWorker() { return worker; } - public Integer getWorkerChangeCount() { - return workerChangeCount; - } - @Override public ServerName getServerName() { return this.crashedServer; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java index d705d9be9915..e7428f3b31c9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java @@ -237,6 +237,7 @@ static void requestLogRoll(final WAL wal) { /** File Extension used while splitting an WAL into regions (HBASE-2312) */ public static final String SPLITTING_EXT = "-splitting"; + // Extension for the WAL where the split failed on one worker and is being retried on another. public static final String RETRYING_EXT = ".retrying"; /** diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java index f053461fb6e9..5186ea3d865a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java @@ -99,7 +99,7 @@ public void teardown() throws Exception { } @Test - public void testRenameWALForRetry() throws Exception { + public void testRenameWALForRetryWALSplit() throws Exception { HRegionServer regionServer = TEST_UTIL.getHBaseCluster().getRegionServer(0); List logDirs = master.getMasterWalManager().getLogDirs(Collections.singleton(regionServer.getServerName())); @@ -134,6 +134,45 @@ public void testRenameWALForRetry() throws Exception { Assert.assertEquals(walFromFileSystem.toString(), newWALPath); } + @Test + public void testRenameWALForFirstRetryWhenWALAlreadyHaveRetryingSuffix() throws Exception { + HRegionServer regionServer = TEST_UTIL.getHBaseCluster().getRegionServer(0); + List logDirs = + master.getMasterWalManager().getLogDirs(Collections.singleton(regionServer.getServerName())); + List list = + SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); + Path testWal = list.get(0).getPath(); + int workerChangeCount = 0; + + String newWALPath = splitWALManager.renameWALForRetry(testWal.toString(), workerChangeCount); + Assert.assertEquals(newWALPath, + testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); + // Let's say that after this step, the SplitWalProcedure and SCP are bypassed. To reach a + // consistent state, the recoverUnknown or schedulerRecovery command will create another SCP and + // SplitWalProcedure. If that also fails on the first attempt + + int workerChangeCountForNewProcedure = 0; + newWALPath = splitWALManager.renameWALForRetry(newWALPath, workerChangeCountForNewProcedure); + Assert.assertEquals(newWALPath, + testWal + RETRYING_EXT + String.format("%03d", workerChangeCountForNewProcedure + 1)); + + List fileStatusesA = + SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); + + Path walFromFileSystem = null; + for (FileStatus wal : fileStatusesA) { + if ( + wal.getPath().toString() + .endsWith(RETRYING_EXT + String.format("%03d", workerChangeCountForNewProcedure + 1)) + ) { + walFromFileSystem = wal.getPath(); + break; + } + } + Assert.assertNotNull(walFromFileSystem); + Assert.assertEquals(walFromFileSystem.toString(), newWALPath); + } + @Test public void testAcquireAndRelease() throws Exception { List testProcedures = new ArrayList<>(); From e5f51ea126c741d165f7de64d66f4c8685da2195 Mon Sep 17 00:00:00 2001 From: ukumawat Date: Tue, 17 Dec 2024 00:10:09 +0530 Subject: [PATCH 4/6] HBASE-28951 remove the counter in procedure and sue wal name only to count retry --- .../server/master/MasterProcedure.proto | 1 - .../hadoop/hbase/master/SplitWALManager.java | 4 +- .../master/procedure/SplitWALProcedure.java | 8 +-- .../hbase/master/TestSplitWALManager.java | 51 ++----------------- 4 files changed, 10 insertions(+), 54 deletions(-) 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 49d01da9a9f8..81d16b2861ca 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -709,7 +709,6 @@ message SplitWALData{ required string wal_path = 1; required ServerName crashed_server=2; optional ServerName worker = 3; - optional uint32 workerChangeCount =4; } message SplitWALRemoteData{ diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index c47b66e4dc04..46665f993635 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -186,7 +186,7 @@ public void addUsedSplitWALWorker(ServerName worker) { splitWorkerAssigner.addUsedWorker(worker); } - public String renameWALForRetry(String walPath, Integer workerChangeCount) throws IOException { + public String renameWALForRetry(String walPath) throws IOException { String originalWALPath; // If the WAL split is retried with another worker, ".retrying-xxx" is appended to the walPath. // In the case where a SplitWalProcedure is bypassed or rolled back after being retried with @@ -194,8 +194,10 @@ public String renameWALForRetry(String walPath, Integer workerChangeCount) throw // SplitWalProcedure (by different SCP) will be created, which will have a workerChangeCount of // 0 but will still have ".retrying-xxx" appended to the walPath. This is why we cannot use the // condition workerChangeCount != 0. + int workerChangeCount = 0; if (walPath.substring(walPath.length() - RETRYING_EXT.length() - 3).startsWith(RETRYING_EXT)) { originalWALPath = walPath.substring(0, walPath.length() - RETRYING_EXT.length() - 3); + workerChangeCount = Integer.parseInt(walPath.substring(walPath.length() - 3)); } else { originalWALPath = walPath; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java index eb96071785db..4bc8bbcb7268 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java @@ -51,7 +51,6 @@ public class SplitWALProcedure private ServerName worker; private ServerName crashedServer; private RetryCounter retryCounter; - private int workerChangeCount = 0; public SplitWALProcedure() { } @@ -90,8 +89,7 @@ protected Flow executeFromState(MasterProcedureEnv env, MasterProcedureProtos.Sp // HBASE-28951 in case of a failure because unresponsive rs, rs might still be processing // the split wal try { - this.walPath = splitWALManager.renameWALForRetry(this.walPath, workerChangeCount); - workerChangeCount++; + this.walPath = splitWALManager.renameWALForRetry(this.walPath); } catch (IOException ioe) { LOG.warn("Failed to rename the splitting wal {}", walPath); setTimeoutForSuspend(env, ioe.getMessage()); @@ -147,8 +145,7 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO super.serializeStateData(serializer); MasterProcedureProtos.SplitWALData.Builder builder = MasterProcedureProtos.SplitWALData.newBuilder(); - builder.setWalPath(walPath).setCrashedServer(ProtobufUtil.toServerName(crashedServer)) - .setWorkerChangeCount(workerChangeCount); + builder.setWalPath(walPath).setCrashedServer(ProtobufUtil.toServerName(crashedServer)); if (worker != null) { builder.setWorker(ProtobufUtil.toServerName(worker)); } @@ -162,7 +159,6 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws serializer.deserialize(MasterProcedureProtos.SplitWALData.class); walPath = data.getWalPath(); crashedServer = ProtobufUtil.toServerName(data.getCrashedServer()); - workerChangeCount = data.getWorkerChangeCount(); if (data.hasWorker()) { worker = ProtobufUtil.toServerName(data.getWorker()); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java index 5186ea3d865a..8e3606cf9222 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java @@ -108,14 +108,13 @@ public void testRenameWALForRetryWALSplit() throws Exception { Path testWal = list.get(0).getPath(); int workerChangeCount = 0; - String newWALPath = splitWALManager.renameWALForRetry(testWal.toString(), workerChangeCount); + String newWALPath = splitWALManager.renameWALForRetry(testWal.toString()); Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); - workerChangeCount = workerChangeCount + 1; + testWal + RETRYING_EXT + String.format("%03d", ++workerChangeCount)); - newWALPath = splitWALManager.renameWALForRetry(newWALPath, workerChangeCount); + newWALPath = splitWALManager.renameWALForRetry(newWALPath); Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); + testWal + RETRYING_EXT + String.format("%03d", ++workerChangeCount)); List fileStatusesA = SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); @@ -123,47 +122,7 @@ public void testRenameWALForRetryWALSplit() throws Exception { Path walFromFileSystem = null; for (FileStatus wal : fileStatusesA) { if ( - wal.getPath().toString() - .endsWith(RETRYING_EXT + String.format("%03d", workerChangeCount + 1)) - ) { - walFromFileSystem = wal.getPath(); - break; - } - } - Assert.assertNotNull(walFromFileSystem); - Assert.assertEquals(walFromFileSystem.toString(), newWALPath); - } - - @Test - public void testRenameWALForFirstRetryWhenWALAlreadyHaveRetryingSuffix() throws Exception { - HRegionServer regionServer = TEST_UTIL.getHBaseCluster().getRegionServer(0); - List logDirs = - master.getMasterWalManager().getLogDirs(Collections.singleton(regionServer.getServerName())); - List list = - SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); - Path testWal = list.get(0).getPath(); - int workerChangeCount = 0; - - String newWALPath = splitWALManager.renameWALForRetry(testWal.toString(), workerChangeCount); - Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", workerChangeCount + 1)); - // Let's say that after this step, the SplitWalProcedure and SCP are bypassed. To reach a - // consistent state, the recoverUnknown or schedulerRecovery command will create another SCP and - // SplitWalProcedure. If that also fails on the first attempt - - int workerChangeCountForNewProcedure = 0; - newWALPath = splitWALManager.renameWALForRetry(newWALPath, workerChangeCountForNewProcedure); - Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", workerChangeCountForNewProcedure + 1)); - - List fileStatusesA = - SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); - - Path walFromFileSystem = null; - for (FileStatus wal : fileStatusesA) { - if ( - wal.getPath().toString() - .endsWith(RETRYING_EXT + String.format("%03d", workerChangeCountForNewProcedure + 1)) + wal.getPath().toString().endsWith(RETRYING_EXT + String.format("%03d", workerChangeCount)) ) { walFromFileSystem = wal.getPath(); break; From 398bc1f21827002701555a3df32c5ab9a2c6cd7c Mon Sep 17 00:00:00 2001 From: ukumawat Date: Sat, 21 Dec 2024 01:01:17 +0530 Subject: [PATCH 5/6] HBASE-28951 introduce and new state to rename the wal --- .../server/master/MasterProcedure.proto | 1 + .../hadoop/hbase/master/SplitWALManager.java | 40 ++++++------ .../master/procedure/SplitWALProcedure.java | 61 ++++++++++++++++--- .../hbase/master/TestSplitWALManager.java | 23 +++---- 4 files changed, 82 insertions(+), 43 deletions(-) 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 81d16b2861ca..fc5d4b4db0c5 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -723,6 +723,7 @@ enum SplitWALState{ ACQUIRE_SPLIT_WAL_WORKER = 1; DISPATCH_WAL_TO_WORKER = 2; RELEASE_SPLIT_WORKER = 3; + RETRY_ON_DIFFERENT_WORKER = 4; } message ClaimReplicationQueuesStateData { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index 46665f993635..5089bc291268 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -21,7 +21,6 @@ import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER; import static org.apache.hadoop.hbase.master.MasterWalManager.META_FILTER; import static org.apache.hadoop.hbase.master.MasterWalManager.NON_META_FILTER; -import static org.apache.hadoop.hbase.wal.AbstractFSWALProvider.RETRYING_EXT; import java.io.IOException; import java.util.Arrays; @@ -186,26 +185,27 @@ public void addUsedSplitWALWorker(ServerName worker) { splitWorkerAssigner.addUsedWorker(worker); } - public String renameWALForRetry(String walPath) throws IOException { - String originalWALPath; - // If the WAL split is retried with another worker, ".retrying-xxx" is appended to the walPath. - // In the case where a SplitWalProcedure is bypassed or rolled back after being retried with - // another worker, the walPath will still have ".retrying-xxx" appended. During recovery, a new - // SplitWalProcedure (by different SCP) will be created, which will have a workerChangeCount of - // 0 but will still have ".retrying-xxx" appended to the walPath. This is why we cannot use the - // condition workerChangeCount != 0. - int workerChangeCount = 0; - if (walPath.substring(walPath.length() - RETRYING_EXT.length() - 3).startsWith(RETRYING_EXT)) { - originalWALPath = walPath.substring(0, walPath.length() - RETRYING_EXT.length() - 3); - workerChangeCount = Integer.parseInt(walPath.substring(walPath.length() - 3)); + /** + * Rename the WAL file at the specified walPath to retry with another worker. Returns true if the + * file is successfully renamed, or if it has already been renamed in previous try. Returns false + * if neither of the files exists. It throws an IOException if got any error while renaming. + */ + public boolean ifExistRenameWALForRetry(String walPath, String postRenameWalPath) + throws IOException { + if (fs.exists(new Path(rootDir, walPath))) { + if (!fs.rename(new Path(rootDir, walPath), new Path(rootDir, postRenameWalPath))) { + throw new IOException("Failed to rename wal " + walPath + " to " + postRenameWalPath); + } + return true; } else { - originalWALPath = walPath; - } - String walNewName = - originalWALPath + RETRYING_EXT + String.format("%03d", (workerChangeCount + 1) % 1000); - if (!fs.rename(new Path(rootDir, walPath), new Path(rootDir, walNewName))) { - throw new IOException("Failed to rename wal " + walPath + " to " + walNewName); + if (fs.exists(new Path(rootDir, postRenameWalPath))) { + LOG.info( + "{} was already renamed in last retry to {}, will continue to acquire new worker to split wal", + walPath, postRenameWalPath); + return true; + } else { + return false; + } } - return walNewName; } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java index 4bc8bbcb7268..1f40f51057ff 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SplitWALProcedure.java @@ -17,6 +17,8 @@ */ package org.apache.hadoop.hbase.master.procedure; +import static org.apache.hadoop.hbase.wal.AbstractFSWALProvider.RETRYING_EXT; + import java.io.IOException; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.ServerName; @@ -86,25 +88,64 @@ protected Flow executeFromState(MasterProcedureEnv env, MasterProcedureProtos.Sp splitWALManager.releaseSplitWALWorker(worker, env.getProcedureScheduler()); if (!finished) { LOG.warn("Failed to split wal {} by server {}, retry...", walPath, worker); - // HBASE-28951 in case of a failure because unresponsive rs, rs might still be processing - // the split wal - try { - this.walPath = splitWALManager.renameWALForRetry(this.walPath); - } catch (IOException ioe) { - LOG.warn("Failed to rename the splitting wal {}", walPath); - setTimeoutForSuspend(env, ioe.getMessage()); - throw new ProcedureSuspendedException(); - } - setNextState(MasterProcedureProtos.SplitWALState.ACQUIRE_SPLIT_WAL_WORKER); + setNextState(MasterProcedureProtos.SplitWALState.RETRY_ON_DIFFERENT_WORKER); return Flow.HAS_MORE_STATE; } ServerCrashProcedure.updateProgress(env, getParentProcId()); return Flow.NO_MORE_STATE; + case RETRY_ON_DIFFERENT_WORKER: + // HBASE-28951: In some cases, a RegionServer (RS) becomes unresponsive, which leads the + // master to mistakenly assume the RS has crashed and mark the SplitWALRemoteProcedure as + // failed. As a result, the WAL splitting task is reassigned to another worker. However, if + // the original worker starts responding again, both RegionServers may attempt to process + // the same WAL at the same time, causing both operations to fail. To prevent this conflict, + // we can "fence" the WAL by renaming it. + try { + String postRenameWalPath = getPostRenameWalPath(); + if (splitWALManager.ifExistRenameWALForRetry(this.walPath, postRenameWalPath)) { + this.walPath = postRenameWalPath; + setNextState(MasterProcedureProtos.SplitWALState.ACQUIRE_SPLIT_WAL_WORKER); + return Flow.HAS_MORE_STATE; + } else { + // The method ifExistRenameWALForRetry will return false if the WAL file at walPath does + // not exist and has not been renamed to postRenameWalPath. This can only happen if the + // WALSplit was already completed before the flow reached this point.. + ServerCrashProcedure.updateProgress(env, getParentProcId()); + return Flow.NO_MORE_STATE; + } + } catch (IOException ioe) { + LOG.warn("Failed to rename the splitting wal {}", walPath); + setTimeoutForSuspend(env, ioe.getMessage()); + throw new ProcedureSuspendedException(); + } + default: throw new UnsupportedOperationException("unhandled state=" + state); } } + private String getPostRenameWalPath() { + // If the WAL split is retried with a different worker, ".retrying-xxx" is appended to the + // walPath. We cannot maintain a workerChangeCount in the SplitWALProcedure class due to the + // following scenario: + // If a SplitWALProcedure is bypassed or rolled back after being retried with another worker, + // the walPath will still have the ".retrying-xxx" suffix. During recovery, a new + // SplitWALProcedure (potentially by a different SCP) will be created. This new procedure will + // have its workerChangeCount initialized to 0, but the walPath will retain the ".retrying-xxx" + // suffix from the previous retry. To handle all these scenarios, we need to move to the next + // count(retrying-xxy) in order to properly fence the last worker. + String originalWALPath, postRenameWalPath; + int workerChangeCount = 0; + if (walPath.substring(walPath.length() - RETRYING_EXT.length() - 3).startsWith(RETRYING_EXT)) { + originalWALPath = walPath.substring(0, walPath.length() - RETRYING_EXT.length() - 3); + workerChangeCount = Integer.parseInt(walPath.substring(walPath.length() - 3)); + } else { + originalWALPath = walPath; + } + return originalWALPath + RETRYING_EXT + String.format("%03d", (workerChangeCount + 1) % 1000); + + } + private void setTimeoutForSuspend(MasterProcedureEnv env, String reason) { if (retryCounter == null) { retryCounter = ProcedureUtil.createRetryCounter(env.getMasterConfiguration()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java index 8e3606cf9222..8c1b40500879 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java @@ -106,30 +106,27 @@ public void testRenameWALForRetryWALSplit() throws Exception { List list = SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); Path testWal = list.get(0).getPath(); - int workerChangeCount = 0; - String newWALPath = splitWALManager.renameWALForRetry(testWal.toString()); - Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", ++workerChangeCount)); - - newWALPath = splitWALManager.renameWALForRetry(newWALPath); - Assert.assertEquals(newWALPath, - testWal + RETRYING_EXT + String.format("%03d", ++workerChangeCount)); + String newWALPath = testWal + RETRYING_EXT + "001"; + boolean result = splitWALManager.ifExistRenameWALForRetry(testWal.toString(), newWALPath); + Assert.assertTrue(result); List fileStatusesA = SplitLogManager.getFileList(TEST_UTIL.getConfiguration(), logDirs, NON_META_FILTER); - Path walFromFileSystem = null; for (FileStatus wal : fileStatusesA) { - if ( - wal.getPath().toString().endsWith(RETRYING_EXT + String.format("%03d", workerChangeCount)) - ) { + if (wal.getPath().toString().endsWith(RETRYING_EXT + "001")) { walFromFileSystem = wal.getPath(); break; } } Assert.assertNotNull(walFromFileSystem); Assert.assertEquals(walFromFileSystem.toString(), newWALPath); + + // if file is already renamed it should return true + boolean resultAfterRename = + splitWALManager.ifExistRenameWALForRetry(testWal.toString(), newWALPath); + Assert.assertTrue(resultAfterRename); } @Test @@ -204,7 +201,7 @@ public void testCreateSplitWALProcedures() throws Exception { Assert.assertFalse(TEST_UTIL.getTestFileSystem().exists(wals[0].getPath())); // Test splitting wal - wals = TEST_UTIL.getTestFileSystem().listStatus(metaWALDir, MasterWalManager.NON_META_FILTER); + wals = TEST_UTIL.getTestFileSystem().listStatus(metaWALDir, NON_META_FILTER); Assert.assertEquals(1, wals.length); testProcedures = splitWALManager.createSplitWALProcedures(Lists.newArrayList(wals[0]), metaServer); From dd576cdbe6070dcc43a10e5238ffc5a207429272 Mon Sep 17 00:00:00 2001 From: ukumawat Date: Wed, 1 Jan 2025 00:58:03 +0530 Subject: [PATCH 6/6] HBASE-28951 added some comments --- .../org/apache/hadoop/hbase/master/SplitWALManager.java | 4 +++- .../org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java index 5089bc291268..9dc5ce70d69d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitWALManager.java @@ -188,7 +188,9 @@ public void addUsedSplitWALWorker(ServerName worker) { /** * Rename the WAL file at the specified walPath to retry with another worker. Returns true if the * file is successfully renamed, or if it has already been renamed in previous try. Returns false - * if neither of the files exists. It throws an IOException if got any error while renaming. + * if neither of the files exists. It throws an IOException if got any error while renaming. This + * method is only called in case of failure on one worker so in case of no failure flow is same as + * old one. */ public boolean ifExistRenameWALForRetry(String walPath, String postRenameWalPath) throws IOException { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java index e7428f3b31c9..723ea16c9457 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java @@ -234,10 +234,14 @@ static void requestLogRoll(final WAL wal) { static final String DEFAULT_PROVIDER_ID = "default"; // Implementation details that currently leak in tests or elsewhere follow - /** File Extension used while splitting an WAL into regions (HBASE-2312) */ + /** + * File Extension used while splitting an WAL into regions (HBASE-2312) This is used with the + * directory name/path + */ public static final String SPLITTING_EXT = "-splitting"; // Extension for the WAL where the split failed on one worker and is being retried on another. + // this is used with the WAL file itself public static final String RETRYING_EXT = ".retrying"; /**