Skip to content

HBASE-23197 'IllegalArgumentException: Wrong FS' on edits replay when… #740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wchevreuil merged 6 commits into from
Nov 23, 2019
Merged

HBASE-23197 'IllegalArgumentException: Wrong FS' on edits replay when… #740

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96f5b1f
HBASE-23197 'IllegalArgumentException: Wrong FS' on edits replay when…
wchevreuil Oct 21, 2019
f0cb94f
Addressing PR comments:
wchevreuil Oct 28, 2019
f1601fd
fixing latest checkstyle issue
wchevreuil Nov 14, 2019
3a5b242
Moved archiving recovered edits to its own method, refactored some te…
wchevreuil Nov 20, 2019
57ab190
adjusting new method javadoc
wchevreuil Nov 20, 2019
c450862
fixing checkstyle and addressing last minor suggestion fom the PR rev…
wchevreuil Nov 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/backup/HFileArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.PathFilter;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.regionserver.HStoreFile;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.CommonFSUtils;
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
import org.apache.hadoop.hbase.util.FSUtils;
import org.apache.hadoop.hbase.util.HFileArchiveUtil;
Expand Down Expand Up @@ -314,9 +316,15 @@ public static void archiveStoreFiles(Configuration conf, FileSystem fs, RegionIn
// build the archive path
if (regionInfo == null || family == null) throw new IOException(
"Need to have a region and a family to archive from.");

Path storeArchiveDir = HFileArchiveUtil.getStoreArchivePath(conf, regionInfo, tableDir, family);

String workingDir = conf.get(CommonFSUtils.HBASE_WAL_DIR);
Path rootDir = null;
if(workingDir == null || !workingDir.startsWith(fs.getScheme())){
Comment thread
joshelser marked this conversation as resolved.
Outdated

@joshelser joshelser Nov 14, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still worries me because if we ever get passed the wrong filesystem, we'll fail in the same way. It's not clear which filesystem is getting passed into this method either (tracing back up through HRegion).

@wchevreuil wchevreuil Nov 16, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So archiveStoreFiles gets called by HRegionFileSystem.removeStoreFiles and MobUtils.removeMobFiles. We are really interested on check HRegionFileSystem.removeStoreFiles, since this is the one that eventually get's passed a different FS from the actual store files one, when called from HRegion.replayRecoveredEditsIfAny. There are then the possible combinations of FS and file types to be archived:

  1. We are archiving recovered edits, wals on different dir but same FS as root dir:
    this workingDir value is non null, but since it doesn't has an fs scheme defined, we can assume it's on the same FS as the passed root dir. In this case, we passed the WAL FS, back there on HRegion.replayRecoveredEditsIfAny, but the WAL FS is same FS as the rootdir, so we are good to go with the normal archive path computed from the root dir value.

  2. We are archiving recovered edits, WALs configured on different FS than the rootdir:
    workinDir value is non null and it does start with the same scheme as the passed FS (since we passed the wal FS here). This will create the path for archiving on the WAL FS, as we are passing an schema + authority format.

  3. Archiving store files with wal dir set to a separate dir on the same FS as root dir:
    Again workingDir is non null, but it will not have the scheme portion, so we set workingDir back to the root dir value, which is what we want for store files. This is the case being tested at TestHRegion.testArchiveRecoveredEditsReplay, that's why we assert the files are on a folder under original archive dir.

  4. Archiving store files, wal dir set to a different FS:
    Again workingDir is non null, but it's scheme is different from the passed FS scheme, so we reset it to the rootdir value.

workingDir = conf.get(HConstants.HBASE_DIR);
}
workingDir = workingDir.substring(workingDir.lastIndexOf("/"));
Comment thread
wchevreuil marked this conversation as resolved.
Outdated
rootDir = new Path(workingDir);
Path storeArchiveDir = HFileArchiveUtil.
getStoreArchivePathForRootDir(rootDir, regionInfo, family);
// make sure we don't archive if we can't and that the archive dir exists
if (!fs.mkdirs(storeArchiveDir)) {
throw new IOException("Could not make archive directory (" + storeArchiveDir + ") for store:"
Expand Down
17 changes: 17 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,23 @@ public static Path getStoreArchivePath(Configuration conf,
return HStore.getStoreHomedir(tableArchiveDir, region, family);
}

/**
* Gets the archive directory under specified root dir. One scenario where this is useful is
* when WAL and root dir are configured under different file systems,
* i.e. root dir on S3 and WALs on HDFS.
* This is mostly useful for archiving recovered edits, when
* <b>hbase.region.archive.recovered.edits</b> is enabled.
* @param Path {@link Configuration} the root dir under which archive path should be created.
* @param region parent region information under which the store currently lives
* @param family name of the family in the store
* @return {@link Path} to the WAL FS directory to archive the given store
* or <tt>null</tt> if it should not be archived
*/
public static Path getStoreArchivePathForRootDir(Path rootDir, RegionInfo region, byte[] family) {
Path tableArchiveDir = getTableArchivePath(rootDir, region.getTable());
return HStore.getStoreHomedir(tableArchiveDir, region, family);
}

/**
* Get the archive directory for a given region under the specified table
* @param tableName the table name. Cannot be null.
Expand Down
37 changes: 37 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/backup/TestHFileArchiving.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.io.IOException;
import java.security.PrivilegedExceptionAction;
Expand All @@ -43,16 +48,19 @@
import org.apache.hadoop.hbase.Stoppable;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.Admin;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.client.Table;
import org.apache.hadoop.hbase.master.HMaster;
import org.apache.hadoop.hbase.master.cleaner.DirScanPool;
import org.apache.hadoop.hbase.master.cleaner.HFileCleaner;
import org.apache.hadoop.hbase.regionserver.ConstantSizeRegionSplitPolicy;
import org.apache.hadoop.hbase.regionserver.HRegion;
import org.apache.hadoop.hbase.regionserver.HRegionServer;
import org.apache.hadoop.hbase.regionserver.HStoreFile;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.MiscTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.CommonFSUtils;
import org.apache.hadoop.hbase.util.FSUtils;
import org.apache.hadoop.hbase.util.HFileArchiveTestingUtil;
import org.apache.hadoop.hbase.util.HFileArchiveUtil;
Expand All @@ -67,6 +75,7 @@
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.rules.TestName;
import org.mockito.ArgumentCaptor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -128,6 +137,34 @@ public static void cleanupTest() throws Exception {
POOL.shutdownNow();
}

@Test
public void testArchiveStoreFilesDifferentFileSystems() throws IOException {
FileSystem mockedFileSystem = mock(FileSystem.class);
UTIL.getConfiguration().set(CommonFSUtils.HBASE_WAL_DIR, "mockFS://mockDir");
Path filePath = new Path("/mockFile");
when(mockedFileSystem.getScheme()).thenReturn("mockFS");
when(mockedFileSystem.mkdirs(any())).thenReturn(true);
when(mockedFileSystem.exists(any())).thenReturn(true);
RegionInfo mockedRegion = mock(RegionInfo.class);
TableName tableName = TableName.valueOf("mockTable");
when(mockedRegion.getTable()).thenReturn(tableName);
when(mockedRegion.getEncodedName()).thenReturn("mocked-region-encoded-name");
Path tableDir = new Path("mockFS://mockDir/tabledir");
byte[] family = Bytes.toBytes("testfamily");
HStoreFile mockedFile = mock(HStoreFile.class);
List<HStoreFile> list = new ArrayList<>();
list.add(mockedFile);
when(mockedFile.getPath()).thenReturn(filePath);
when(mockedFileSystem.rename(any(),any())).thenReturn(true);
HFileArchiver.archiveStoreFiles(UTIL.getConfiguration(), mockedFileSystem, mockedRegion,
tableDir, family, list);
ArgumentCaptor<Path> pathCaptor = ArgumentCaptor.forClass(Path.class);
verify(mockedFileSystem, times(2)).rename(pathCaptor.capture(), any());
assertTrue(pathCaptor.getAllValues().get(0).toString().
equals("/mockDir/archive/data/default/mockTable/"
+ "mocked-region-encoded-name/testfamily/mockFile"));
}

@Test
public void testRemoveRegionDirOnArchive() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());
Expand Down