From 6a794e397bef962d2901eb5bb0cba0a7c50f35e8 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 | 119 +++++++----------- .../access/SnapshotScannerHDFSAclHelper.java | 4 +- .../hbase/master/TestMasterFileSystem.java | 29 +---- .../procedure/TestDeleteTableProcedure.java | 66 ---------- 5 files changed, 55 insertions(+), 193 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 55e32126bdf..441fddbfe7c 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 @@ -22,7 +22,6 @@ package org.apache.hadoop.hbase.master.procedure; import java.io.IOException; import java.util.ArrayList; import java.util.List; -import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseIOException; @@ -316,41 +315,22 @@ public class CreateTableProcedure 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 8322383686c..46144867d54 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,7 @@ package org.apache.hadoop.hbase.master.procedure; 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; @@ -52,11 +49,12 @@ import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils; + import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos; import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.DeleteTableState; -import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils; @InterfaceAudience.Private public class DeleteTableProcedure @@ -278,91 +276,58 @@ public class DeleteTableProcedure 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); - } - 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 regions from FS (temp directory) - if (archive) { - List regionDirList = new ArrayList<>(); - for (RegionInfo region : regions) { - if (RegionReplicaUtil.isDefaultReplica(region)) { - regionDirList.add(FSUtils.getRegionDirFromTableDir(tempTableDir, region)); - List mergeRegions = MetaTableAccessor + // Archive regions from FS (temp directory) + if (archive) { + List regionDirList = new ArrayList<>(); + for (RegionInfo region : regions) { + if (RegionReplicaUtil.isDefaultReplica(region)) { + regionDirList.add(FSUtils.getRegionDirFromTableDir(tableDir, region)); + List mergeRegions = MetaTableAccessor .getMergeRegions(env.getMasterServices().getConnection(), region.getRegionName()); - if (!CollectionUtils.isEmpty(mergeRegions)) { - mergeRegions.stream() - .forEach(r -> regionDirList.add(FSUtils.getRegionDirFromTableDir(tempTableDir, r))); + if (!CollectionUtils.isEmpty(mergeRegions)) { + mergeRegions.stream().forEach( + r -> regionDirList.add(FSUtils.getRegionDirFromTableDir(tableDir, r))); + } } } + HFileArchiver + .archiveRegions(env.getMasterConfiguration(), fs, mfs.getRootDir(), tableDir, + regionDirList); + if (!regionDirList.isEmpty()) { + LOG.debug("Archived {} regions", tableName); + } } - HFileArchiver.archiveRegions(env.getMasterConfiguration(), fs, mfs.getRootDir(), tempTableDir, - regionDirList); - if (!regionDirList.isEmpty()) { - LOG.debug("Archived {} regions", tableName); + + // 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 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 table directory from FS + if (!fs.delete(tableDir, true) && fs.exists(tableDir)) { + throw new IOException("Couldn't delete " + tableDir); } - } - // 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 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); + } } } 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 ffe8dab579e..fbdc6381d7a 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 @@ -478,8 +478,8 @@ public class SnapshotScannerHDFSAclHelper implements Closeable { */ 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 63d303dc710..1461c0612b5 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.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.AfterClass; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -85,7 +83,7 @@ public class TestMasterFileSystem { } @Test - public void testCheckTempDir() throws Exception { + public void testCheckNoTempDir() throws Exception { final MasterFileSystem masterFileSystem = UTIL.getMiniHBaseCluster().getMaster().getMasterFileSystem(); @@ -110,28 +108,13 @@ public class TestMasterFileSystem { // 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 1dd7dc4c620..9367a575958 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 class TestDeleteTableProcedure extends TestTableDDLProcedureBase { 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)); - } - } }