[GitHub] [hbase] Apache9 commented on a change in pull request #3574: HBASE-26187 Write straight into the store directory when Splitting an…

[GitBox <gi...@apache.org>]

Apache9 commented on a change in pull request #3574:
URL: https://github.com/apache/hbase/pull/3574#discussion_r686450336



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java
##########
@@ -638,47 +602,33 @@ void cleanupDaughterRegion(final RegionInfo regionInfo) throws IOException {
    */
   public Path commitDaughterRegion(final RegionInfo regionInfo)
       throws IOException {
-    Path regionDir = new Path(this.tableDir, regionInfo.getEncodedName());
-    Path daughterTmpDir = this.getSplitsDir(regionInfo);
-
-    if (fs.exists(daughterTmpDir)) {
-
+    Path regionDir = this.getSplitsDir(regionInfo);
+    if (fs.exists(regionDir)) {
       // Write HRI to a file in case we need to recover hbase:meta
-      Path regionInfoFile = new Path(daughterTmpDir, REGION_INFO_FILE);
+      Path regionInfoFile = new Path(regionDir, REGION_INFO_FILE);
       byte[] regionInfoContent = getRegionInfoFileContent(regionInfo);
       writeRegionInfoFileContent(conf, fs, regionInfoFile, regionInfoContent);
-
-      // Move the daughter temp dir to the table dir
-      if (!rename(daughterTmpDir, regionDir)) {
-        throw new IOException("Unable to rename " + daughterTmpDir + " to " + regionDir);
-      }
     }
 
     return regionDir;
   }
 
   /**
-   * Create the region splits directory.
+   * Creates region split daughter directories under the table dir.
    */
   public void createSplitsDir(RegionInfo daughterA, RegionInfo daughterB) throws IOException {
     Path splitdir = getSplitsDir();
-    if (fs.exists(splitdir)) {
-      LOG.info("The " + splitdir + " directory exists.  Hence deleting it to recreate it");
-      if (!deleteDir(splitdir)) {
-        throw new IOException("Failed deletion of " + splitdir + " before creating them again.");
-      }
-    }
     // splitDir doesn't exists now. No need to do an exists() call for it.
-    if (!createDir(splitdir)) {
-      throw new IOException("Failed create of " + splitdir);
+    if (!fs.exists(splitdir)) {

Review comment:
       So the split directory is now the table directory, why we still need to check existence here? And IMO here we should not use 'splitDir' and 'mergeDir' in code any more, as now there are only region directories...

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java
##########
@@ -638,47 +602,33 @@ void cleanupDaughterRegion(final RegionInfo regionInfo) throws IOException {
    */
   public Path commitDaughterRegion(final RegionInfo regionInfo)
       throws IOException {
-    Path regionDir = new Path(this.tableDir, regionInfo.getEncodedName());
-    Path daughterTmpDir = this.getSplitsDir(regionInfo);
-
-    if (fs.exists(daughterTmpDir)) {
-
+    Path regionDir = this.getSplitsDir(regionInfo);
+    if (fs.exists(regionDir)) {
       // Write HRI to a file in case we need to recover hbase:meta
-      Path regionInfoFile = new Path(daughterTmpDir, REGION_INFO_FILE);
+      Path regionInfoFile = new Path(regionDir, REGION_INFO_FILE);
       byte[] regionInfoContent = getRegionInfoFileContent(regionInfo);
       writeRegionInfoFileContent(conf, fs, regionInfoFile, regionInfoContent);
-
-      // Move the daughter temp dir to the table dir
-      if (!rename(daughterTmpDir, regionDir)) {
-        throw new IOException("Unable to rename " + daughterTmpDir + " to " + regionDir);
-      }
     }
 
     return regionDir;
   }
 
   /**
-   * Create the region splits directory.
+   * Creates region split daughter directories under the table dir.
    */
   public void createSplitsDir(RegionInfo daughterA, RegionInfo daughterB) throws IOException {
     Path splitdir = getSplitsDir();
-    if (fs.exists(splitdir)) {
-      LOG.info("The " + splitdir + " directory exists.  Hence deleting it to recreate it");
-      if (!deleteDir(splitdir)) {
-        throw new IOException("Failed deletion of " + splitdir + " before creating them again.");
-      }
-    }
     // splitDir doesn't exists now. No need to do an exists() call for it.

Review comment:
       This comment is incorrect now?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java
##########
@@ -572,51 +572,15 @@ public void removeStoreFiles(String familyName, Collection<HStoreFile> storeFile
   // ===========================================================================
   //  Splits Helpers
   // ===========================================================================
-  /** @return {@link Path} to the temp directory used during split operations */
+  /** @return {@link Path} to the table directory for split daughters */
   Path getSplitsDir() {

Review comment:
       Do we still need this method?

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionFileSystem.java
##########
@@ -380,4 +382,190 @@ public void testTempAndCommit() throws IOException {
 
     fs.delete(rootDir, true);
   }
+
+  @Test
+  public void testSplitStoreDir() throws Exception {

Review comment:
       Better place the below methods to a different test class? And is it possible to only start mini cluster once? Now every test method needs to start and then stop the mini cluster, which is time consuming...




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org