Apache9 commented on a change in pull request #3749:
URL: https://github.com/apache/hbase/pull/3749#discussion_r730410581
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -326,7 +326,7 @@ public static String createHFileLinkName(final TableName
tableName,
* @return true if the file is created, otherwise the file exists.
* @throws IOException on file or parent directory creation failure
*/
- public static boolean create(final Configuration conf, final FileSystem fs,
+ public static String create(final Configuration conf, final FileSystem fs,
Review comment:
We need to change the javadoc?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -347,7 +347,7 @@ public static boolean create(final Configuration conf,
final FileSystem fs,
* @return true if the file is created, otherwise the file exists.
* @throws IOException on file or parent directory creation failure
*/
- public static boolean create(final Configuration conf, final FileSystem fs,
+ public static String create(final Configuration conf, final FileSystem fs,
Review comment:
DItto.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -392,7 +392,7 @@ public static boolean create(final Configuration conf,
final FileSystem fs,
* @return true if the file is created, otherwise the file exists.
* @throws IOException on file or parent directory creation failure
*/
- public static boolean create(final Configuration conf, final FileSystem fs,
+ public static String create(final Configuration conf, final FileSystem fs,
Review comment:
Ditto.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -464,7 +448,7 @@ public static boolean createFromHFileLink(final
Configuration conf, final FileSy
* @return true if the file is created, otherwise the file exists.
* @throws IOException on file or parent directory creation failure
*/
- public static boolean createFromHFileLink(final Configuration conf, final
FileSystem fs,
+ public static String createFromHFileLink(final Configuration conf, final
FileSystem fs,
Review comment:
Ditto.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/SnapshotStoreFileTracker.java
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver.storefiletracker;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.regionserver.StoreContext;
+import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
+import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
+import org.apache.yetus.audience.InterfaceAudience;
+
+import java.io.IOException;
+import java.util.List;
+
+/**
+ * Extends MigrationStoreFileTracker for Snapshot restore/clone specific case.
+ * When restoring/cloning snapshots, new regions are created with reference
files to the
+ * original regions files. This work is done in snapshot specific classes. We
need to somehow
+ * initialize these reference files in the configured StoreFileTracker. Once
snapshot logic has
+ * cloned the store dir and created the references, it should set the list of
reference files in
+ * <code>SourceTracker.setReferenceFiles</code> then invoke <code>load</code>
method.
+ * <p/>
+ */
[email protected]
+public class SnapshotStoreFileTracker extends MigrationStoreFileTracker {
Review comment:
Please see this post:
https://issues.apache.org/jira/browse/HBASE-26280?focusedCommentId=17414894&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17414894
I think the decision was the snapshot is just a set of HFiles, it just
records the Store File Tracker implementation in the table descriptor, but we
will not use it to record the store files in the snapshot. So I do not think we
need the 'src' store file tracker here. We just get the files for a store, and
then insert it into the target store file tracker. Not sure if I understand
correctly here.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -370,7 +370,7 @@ public static boolean create(final Configuration conf,
final FileSystem fs,
* @return true if the file is created, otherwise the file exists.
* @throws IOException on file or parent directory creation failure
*/
- public static boolean create(final Configuration conf, final FileSystem fs,
+ public static String create(final Configuration conf, final FileSystem fs,
Review comment:
Ditto.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -453,56 +453,24 @@ private void postCloneSnapshot(final MasterProcedureEnv
env)
List<RegionInfo> 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());
- if (CommonFSUtils.isExists(mfs.getFileSystem(), tempTableDir)) {
+ final Path tableDir = CommonFSUtils.getTableDir(mfs.getRootDir(),
tableDescriptor.getTableName());
+ if (CommonFSUtils.isExists(mfs.getFileSystem(), tableDir)) {
// if the region dirs exist, will cause exception and unlimited retry
(see HBASE-24546)
- LOG.warn("temp table dir already exists on disk: {}, will be deleted.",
tempTableDir);
- CommonFSUtils.deleteDirectory(mfs.getFileSystem(), tempTableDir);
+ LOG.warn("temp table dir already exists on disk: {}, will be deleted.",
tableDir);
+ CommonFSUtils.deleteDirectory(mfs.getFileSystem(), tableDir);
Review comment:
This is not a temp directory, so is it still safe to just delete it? I'm
not sure, just asking...
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -453,56 +453,24 @@ private void postCloneSnapshot(final MasterProcedureEnv
env)
List<RegionInfo> newRegions,
final CreateHdfsRegions hdfsRegionHandler) throws IOException {
final MasterFileSystem mfs = env.getMasterServices().getMasterFileSystem();
- final Path tempdir = mfs.getTempDir();
Review comment:
+1, I think this is similiar to the merge/split scenario. There is no
reason that we can not do this. And in #3716 , @frostruan is trying to
implement the snapshot operationby proc-v2, so I think we could implement the
clean up logic in the rollback of the procedure.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/RestoreSnapshotHelper.java
##########
@@ -540,21 +567,39 @@ private void restoreRegion(final RegionInfo regionInfo,
HFileArchiver.archiveFamilyByFamilyDir(fs, conf, regionInfo,
familyDir, family);
fs.delete(familyDir, true);
}
+
+ SnapshotStoreFileTracker tracker = (SnapshotStoreFileTracker)
+ StoreFileTrackerFactory.create(conf, true,
+ StoreContext.getBuilder().withFamilyStoreDirectoryPath(familyDir).
+ withRegionFileSystem(regionFS).build());
+
+ //simply reset list of tracked files with the matching files
+ // and the extra one present in the snapshot
+ tracker.getSourceTracker().setReferenceFiles(filesToTrack);
Review comment:
So why not just create the dest store file tracker and call its set
method with filesToTrack?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -29,6 +29,7 @@
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.client.RegionInfoBuilder;
import org.apache.hadoop.hbase.mob.MobConstants;
+import org.apache.hadoop.hbase.mob.MobUtils;
Review comment:
Where do we use this class?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/RestoreSnapshotHelper.java
##########
@@ -540,21 +567,39 @@ private void restoreRegion(final RegionInfo regionInfo,
HFileArchiver.archiveFamilyByFamilyDir(fs, conf, regionInfo,
familyDir, family);
fs.delete(familyDir, true);
}
+
+ SnapshotStoreFileTracker tracker = (SnapshotStoreFileTracker)
+ StoreFileTrackerFactory.create(conf, true,
+ StoreContext.getBuilder().withFamilyStoreDirectoryPath(familyDir).
+ withRegionFileSystem(regionFS).build());
+
+ //simply reset list of tracked files with the matching files
+ // and the extra one present in the snapshot
+ tracker.getSourceTracker().setReferenceFiles(filesToTrack);
+ tracker.load();
}
// Add families not present in the table
for (Map.Entry<String, List<SnapshotRegionManifest.StoreFile>> familyEntry:
snapshotFiles.entrySet()) {
Path familyDir = new Path(regionDir, familyEntry.getKey());
+ SnapshotStoreFileTracker tracker = (SnapshotStoreFileTracker)
+ StoreFileTrackerFactory.create(conf, true,
+ StoreContext.getBuilder().withFamilyStoreDirectoryPath(familyDir).
+ withRegionFileSystem(regionFS).build());
+ List<StoreFileInfo> files = new ArrayList<>();
if (!fs.mkdirs(familyDir)) {
throw new IOException("Unable to create familyDir=" + familyDir);
}
for (SnapshotRegionManifest.StoreFile storeFile: familyEntry.getValue())
{
LOG.trace("Adding HFileLink (Not present in the table) " +
storeFile.getName()
+ " of snapshot " + snapshotName + " to table=" + tableName);
- restoreStoreFile(familyDir, regionInfo, storeFile, createBackRefs);
+ String fileName = restoreStoreFile(familyDir, regionInfo, storeFile,
createBackRefs);
+ files.add(new StoreFileInfo(conf, fs, new Path(familyDir, fileName),
true));
}
+ tracker.getSourceTracker().setReferenceFiles(files);
Review comment:
Ditto.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/HFileLink.java
##########
@@ -429,25 +431,7 @@ public static boolean create(final Configuration conf,
final FileSystem fs,
}
throw e;
}
- }
-
- /**
- * Create a new HFileLink starting from a hfileLink name
- *
- * <p>It also adds a back-reference to the hfile back-reference directory
- * to simplify the reference-count and the cleaning process.
- *
- * @param conf {@link Configuration} to read for the archive directory name
- * @param fs {@link FileSystem} on which to write the HFileLink
- * @param dstFamilyPath - Destination path (table/region/cf/)
- * @param hfileLinkName - HFileLink name (it contains hfile-region-table)
- * @return true if the file is created, otherwise the file exists.
- * @throws IOException on file or parent directory creation failure
- */
- public static boolean createFromHFileLink(final Configuration conf, final
FileSystem fs,
- final Path dstFamilyPath, final String hfileLinkName)
- throws IOException {
- return createFromHFileLink(conf, fs, dstFamilyPath, hfileLinkName, true);
+ throw new IOException("File link=" + name + " already exists under " +
dstFamilyPath + " folder.");
Review comment:
We changed the behavior here? In the past it could return false but now
it will throw an exception?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
