From d00b5faade4e40f2127fe2b18bf97dae69ce33f6 Mon Sep 17 00:00:00 2001 From: Wellington Ramos Chevreuil Date: Fri, 19 Nov 2021 12:16:29 +0000 Subject: [PATCH] =?UTF-8?q?HBASE-26454=20CreateTableProcedure=20still=20re?= =?UTF-8?q?lies=20on=20temp=20dir=20and=20renames=E2=80=A6=20(#3845)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Duo Zhang --- .../procedure/CreateTableProcedure.java | 30 +----- .../procedure/DeleteTableProcedure.java | 95 ++++++------------- .../access/SnapshotScannerHDFSAclHelper.java | 4 +- .../hbase/master/TestMasterFileSystem.java | 29 ++---- .../procedure/TestDeleteTableProcedure.java | 66 ------------- 5 files changed, 43 insertions(+), 181 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java index 0a6a469328a7..d77b95f186d4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java @@ -23,7 +23,6 @@ import java.util.ArrayList; import java.util.List; import java.util.function.Supplier; -import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseIOException; @@ -336,41 +335,22 @@ protected static List createFsLayout(final MasterProcedureEnv env, final TableDescriptor tableDescriptor, List newRegions, final CreateHdfsRegions hdfsRegionHandler) throws IOException { final MasterFileSystem mfs = env.getMasterServices().getMasterFileSystem(); - final Path tempdir = mfs.getTempDir(); // 1. Create Table Descriptor // using a copy of descriptor, table will be created enabling first - final Path tempTableDir = CommonFSUtils.getTableDir(tempdir, tableDescriptor.getTableName()); + final Path tableDir = CommonFSUtils.getTableDir(mfs.getRootDir(), + tableDescriptor.getTableName()); ((FSTableDescriptors)(env.getMasterServices().getTableDescriptors())) - .createTableDescriptorForTableDirectory(tempTableDir, tableDescriptor, false); + .createTableDescriptorForTableDirectory( + tableDir, tableDescriptor, false); // 2. Create Regions - newRegions = hdfsRegionHandler.createHdfsRegions(env, tempdir, + newRegions = hdfsRegionHandler.createHdfsRegions(env, mfs.getRootDir(), tableDescriptor.getTableName(), newRegions); - // 3. Move Table temp directory to the hbase root location - moveTempDirectoryToHBaseRoot(env, tableDescriptor, tempTableDir); - return newRegions; } - protected static void moveTempDirectoryToHBaseRoot( - final MasterProcedureEnv env, - final TableDescriptor tableDescriptor, - final Path tempTableDir) throws IOException { - final MasterFileSystem mfs = env.getMasterServices().getMasterFileSystem(); - final Path tableDir = - CommonFSUtils.getTableDir(mfs.getRootDir(), tableDescriptor.getTableName()); - FileSystem fs = mfs.getFileSystem(); - if (!fs.delete(tableDir, true) && fs.exists(tableDir)) { - throw new IOException("Couldn't delete " + tableDir); - } - if (!fs.rename(tempTableDir, tableDir)) { - throw new IOException("Unable to move table from temp=" + tempTableDir + - " to hbase root=" + tableDir); - } - } - protected static List addTableToMeta(final MasterProcedureEnv env, final TableDescriptor tableDescriptor, final List regions) throws IOException { assert (regions != null && regions.size() > 0) : "expected at least 1 region, got " + regions; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteTableProcedure.java index 80dddc7ccda1..297efc240fca 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteTableProcedure.java @@ -20,10 +20,8 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; -import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.MetaTableAccessor; @@ -277,82 +275,49 @@ protected static void deleteFromFs(final MasterProcedureEnv env, final boolean archive) throws IOException { final MasterFileSystem mfs = env.getMasterServices().getMasterFileSystem(); final FileSystem fs = mfs.getFileSystem(); - final Path tempdir = mfs.getTempDir(); final Path tableDir = CommonFSUtils.getTableDir(mfs.getRootDir(), tableName); - final Path tempTableDir = CommonFSUtils.getTableDir(tempdir, tableName); if (fs.exists(tableDir)) { - // Ensure temp exists - if (!fs.exists(tempdir) && !fs.mkdirs(tempdir)) { - throw new IOException("HBase temp directory '" + tempdir + "' creation failure."); - } - - // Ensure parent exists - if (!fs.exists(tempTableDir.getParent()) && !fs.mkdirs(tempTableDir.getParent())) { - throw new IOException("HBase temp directory '" + tempdir + "' creation failure."); - } - - if (fs.exists(tempTableDir)) { - // TODO - // what's in this dir? something old? probably something manual from the user... - // let's get rid of this stuff... - FileStatus[] files = fs.listStatus(tempTableDir); - if (files != null && files.length > 0) { - List regionDirList = Arrays.stream(files) - .filter(FileStatus::isDirectory) - .map(FileStatus::getPath) - .collect(Collectors.toList()); - HFileArchiver.archiveRegions(env.getMasterConfiguration(), fs, mfs.getRootDir(), - tempTableDir, regionDirList); + // Archive regions from FS (temp directory) + if (archive) { + List regionDirList = regions.stream().filter(RegionReplicaUtil::isDefaultReplica) + .map(region -> + FSUtils.getRegionDirFromTableDir(tableDir, region)).collect(Collectors.toList()); + HFileArchiver + .archiveRegions(env.getMasterConfiguration(), fs, mfs.getRootDir(), tableDir, + regionDirList); + if (!regionDirList.isEmpty()) { + LOG.debug("Archived {} regions", tableName); } - fs.delete(tempTableDir, true); } - // Move the table in /hbase/.tmp - if (!fs.rename(tableDir, tempTableDir)) { - throw new IOException("Unable to move '" + tableDir + "' to temp '" + tempTableDir + "'"); + // Archive mob data + Path mobTableDir = + CommonFSUtils.getTableDir(new Path(mfs.getRootDir(), MobConstants.MOB_DIR_NAME), tableName); + Path regionDir = new Path(mobTableDir, MobUtils.getMobRegionInfo(tableName).getEncodedName()); + if (fs.exists(regionDir)) { + HFileArchiver.archiveRegion(fs, mfs.getRootDir(), mobTableDir, regionDir); } - } - // Archive regions from FS (temp directory) - if (archive) { - List regionDirList = regions.stream().filter(RegionReplicaUtil::isDefaultReplica) - .map(region -> FSUtils.getRegionDirFromTableDir(tempTableDir, region)) - .collect(Collectors.toList()); - HFileArchiver.archiveRegions(env.getMasterConfiguration(), fs, mfs.getRootDir(), tempTableDir, - regionDirList); - if (!regionDirList.isEmpty()) { - LOG.debug("Archived {} regions", tableName); + // Delete table directory from FS + if (!fs.delete(tableDir, true) && fs.exists(tableDir)) { + throw new IOException("Couldn't delete " + tableDir); } - } - // Archive mob data - Path mobTableDir = - CommonFSUtils.getTableDir(new Path(mfs.getRootDir(), MobConstants.MOB_DIR_NAME), tableName); - Path regionDir = - new Path(mobTableDir, MobUtils.getMobRegionInfo(tableName).getEncodedName()); - if (fs.exists(regionDir)) { - HFileArchiver.archiveRegion(fs, mfs.getRootDir(), mobTableDir, regionDir); - } - - // Delete table directory from FS (temp directory) - if (!fs.delete(tempTableDir, true) && fs.exists(tempTableDir)) { - throw new IOException("Couldn't delete " + tempTableDir); - } - - // Delete the table directory where the mob files are saved - if (mobTableDir != null && fs.exists(mobTableDir)) { - if (!fs.delete(mobTableDir, true)) { - throw new IOException("Couldn't delete mob dir " + mobTableDir); + // Delete the table directory where the mob files are saved + if (mobTableDir != null && fs.exists(mobTableDir)) { + if (!fs.delete(mobTableDir, true)) { + throw new IOException("Couldn't delete mob dir " + mobTableDir); + } } - } - // Delete the directory on wal filesystem - FileSystem walFs = mfs.getWALFileSystem(); - Path tableWALDir = CommonFSUtils.getWALTableDir(env.getMasterConfiguration(), tableName); - if (walFs.exists(tableWALDir) && !walFs.delete(tableWALDir, true)) { - throw new IOException("Couldn't delete table dir on wal filesystem" + tableWALDir); + // Delete the directory on wal filesystem + FileSystem walFs = mfs.getWALFileSystem(); + Path tableWALDir = CommonFSUtils.getWALTableDir(env.getMasterConfiguration(), tableName); + if (walFs.exists(tableWALDir) && !walFs.delete(tableWALDir, true)) { + throw new IOException("Couldn't delete table dir on wal filesystem" + tableWALDir); + } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java index 2f00b1e7a1d2..53d9970e09df 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java @@ -474,8 +474,8 @@ List getNamespaceRootPaths(String namespace) { */ List getTableRootPaths(TableName tableName, boolean includeSnapshotPath) throws IOException { - List paths = Lists.newArrayList(pathHelper.getTmpTableDir(tableName), - pathHelper.getDataTableDir(tableName), pathHelper.getMobTableDir(tableName), + List paths = Lists.newArrayList(pathHelper.getDataTableDir(tableName), + pathHelper.getMobTableDir(tableName), pathHelper.getArchiveTableDir(tableName)); if (includeSnapshotPath) { paths.addAll(getTableSnapshotPaths(tableName)); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFileSystem.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFileSystem.java index 60e46052a038..f08462424a50 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFileSystem.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFileSystem.java @@ -18,8 +18,7 @@ package org.apache.hadoop.hbase.master; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertFalse; import java.util.List; import org.apache.hadoop.fs.FileSystem; @@ -33,7 +32,6 @@ import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CommonFSUtils; -import org.apache.hadoop.hbase.util.HFileArchiveTestingUtil; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -85,7 +83,7 @@ public void testFsUriSetProperly() throws Exception { } @Test - public void testCheckTempDir() throws Exception { + public void testCheckNoTempDir() throws Exception { final MasterFileSystem masterFileSystem = UTIL.getMiniHBaseCluster().getMaster().getMasterFileSystem(); @@ -110,28 +108,13 @@ public void testCheckTempDir() throws Exception { // disable the table so that we can manipulate the files UTIL.getAdmin().disableTable(tableName); - final Path tableDir = CommonFSUtils.getTableDir(masterFileSystem.getRootDir(), tableName); final Path tempDir = masterFileSystem.getTempDir(); - final Path tempTableDir = CommonFSUtils.getTableDir(tempDir, tableName); + final Path tempNsDir = CommonFSUtils.getNamespaceDir(tempDir, + tableName.getNamespaceAsString()); final FileSystem fs = masterFileSystem.getFileSystem(); - // move the table to the temporary directory - if (!fs.rename(tableDir, tempTableDir)) { - fail(); - } - - masterFileSystem.checkTempDir(tempDir, UTIL.getConfiguration(), fs); - - // check if the temporary directory exists and is empty - assertTrue(fs.exists(tempDir)); - assertEquals(0, fs.listStatus(tempDir).length); - - // check for the existence of the archive directory - for (HRegion region : regions) { - Path archiveDir = HFileArchiveTestingUtil.getRegionArchiveDir(UTIL.getConfiguration(), - region); - assertTrue(fs.exists(archiveDir)); - } + // checks the temporary directory does not exist + assertFalse(fs.exists(tempNsDir)); UTIL.deleteTable(tableName); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestDeleteTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestDeleteTableProcedure.java index 1dd7dc4c6206..9367a575958b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestDeleteTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestDeleteTableProcedure.java @@ -17,34 +17,23 @@ */ package org.apache.hadoop.hbase.master.procedure; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.List; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.FileUtil; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotDisabledException; import org.apache.hadoop.hbase.TableNotFoundException; import org.apache.hadoop.hbase.client.RegionInfo; -import org.apache.hadoop.hbase.client.Table; -import org.apache.hadoop.hbase.master.MasterFileSystem; import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; -import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; -import org.apache.hadoop.hbase.util.CommonFSUtils; -import org.apache.hadoop.hbase.util.HFileArchiveTestingUtil; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -186,59 +175,4 @@ public void testRecoveryAndDoubleExecution() throws Exception { MasterProcedureTestingUtility.validateTableDeletion(getMaster(), tableName); } - - @Test - public void testDeleteWhenTempDirIsNotEmpty() throws Exception { - final TableName tableName = TableName.valueOf(name.getMethodName()); - final String FAM = "fam"; - final byte[][] splitKeys = new byte[][] { - Bytes.toBytes("b"), Bytes.toBytes("c"), Bytes.toBytes("d") - }; - - // create the table - MasterProcedureTestingUtility.createTable( - getMasterProcedureExecutor(), tableName, splitKeys, FAM); - - // get the current store files for the regions - List regions = UTIL.getHBaseCluster().getRegions(tableName); - // make sure we have 4 regions serving this table - assertEquals(4, regions.size()); - - // load the table - try (Table table = UTIL.getConnection().getTable(tableName)) { - UTIL.loadTable(table, Bytes.toBytes(FAM)); - } - - // disable the table so that we can manipulate the files - UTIL.getAdmin().disableTable(tableName); - - final MasterFileSystem masterFileSystem = - UTIL.getMiniHBaseCluster().getMaster().getMasterFileSystem(); - final Path tableDir = CommonFSUtils.getTableDir(masterFileSystem.getRootDir(), tableName); - final Path tempDir = masterFileSystem.getTempDir(); - final Path tempTableDir = CommonFSUtils.getTableDir(tempDir, tableName); - final FileSystem fs = masterFileSystem.getFileSystem(); - - // copy the table to the temporary directory to make sure the temp directory is not empty - if (!FileUtil.copy(fs, tableDir, fs, tempTableDir, false, UTIL.getConfiguration())) { - fail(); - } - - // delete the table - final ProcedureExecutor procExec = getMasterProcedureExecutor(); - long procId = ProcedureTestingUtility.submitAndWait(procExec, - new DeleteTableProcedure(procExec.getEnvironment(), tableName)); - ProcedureTestingUtility.assertProcNotFailed(procExec, procId); - MasterProcedureTestingUtility.validateTableDeletion(getMaster(), tableName); - - // check if the temporary directory is deleted - assertFalse(fs.exists(tempTableDir)); - - // check for the existence of the archive directory - for (HRegion region : regions) { - Path archiveDir = HFileArchiveTestingUtil.getRegionArchiveDir(UTIL.getConfiguration(), - region); - assertTrue(fs.exists(archiveDir)); - } - } }