For #1, note that the code is in else block. Meaning tableName table doesn't exist.
cloneTableSchema() is needed because table name would be different. For #4, is checkAndDeleteFiles() a hot spot ? If not, there is not much gain in refactoring. Cheers On Sat, Nov 7, 2015 at 10:21 AM, WangYQ <[email protected]> wrote: > 1. i download the src code from: > http://mirrors.hust.edu.cn/apache/hbase/0.98.15/ > class: org.apache.hadoop.hbase.master.snapshot.SnapshotManager > method: public void restoreSnapshot(SnapshotDescription reqSnapshot) > throws IOException > line: 725 > code: > > > if (cpHost != null) { > cpHost.postRestoreSnapshot(reqSnapshot, snapshotTableDesc); > } > } else { > HTableDescriptor htd = > RestoreSnapshotHelper.cloneTableSchema(snapshotTableDesc, tableName); > if (cpHost != null) { > cpHost.preCloneSnapshot(reqSnapshot, htd); > } > cloneSnapshot(fsSnapshot, htd); > LOG.info("Clone snapshot=" + fsSnapshot.getName() + " as table=" + > tableName); > > > if (cpHost != null) { > cpHost.postCloneSnapshot(reqSnapshot, htd); > } > > i think the clone: > HTableDescriptor htd = > RestoreSnapshotHelper.cloneTableSchema(snapshotTableDesc, tableName); > can be removed. > > > 2. this one is similar to the one in HBaseAdmin.getTableDecsriptor > 3. add method 'isSnapShotExists' in HBaseAdmin to help us to know if > there is a snopshot on HDFS, just like HBaseAdmin.tableExists, > before we create a snopshot of a table, we can check if there is a > snopshot with same name > 4. simplify the code > version: hbase 0.98.15 > class: org.apache.hadoop.hbase.master.cleaner.CleanerChore > method:private boolean checkAndDeleteFiles(List<FileStatus> files) > line: 235-243 > code: > > > // trace which cleaner is holding on to each file > if (LOG.isTraceEnabled()) { > ImmutableSet<FileStatus> filteredFileSet = > ImmutableSet.copyOf(filteredFiles); > for (FileStatus file : deletableValidFiles) { > if (!filteredFileSet.contains(file)) { > LOG.trace(file.getPath() + " is not deletable according to:" + > cleaner); > } > } > } > i think these code can use set's difference operation instead, which > is more straightforward > > > At 2015-11-07 21:10:21, "Ted Yu" <[email protected]> wrote: > >For #1, I took a look at current 0.98 code but didn't find it. > >Can you double check and tell us the updated line number ? > > > >For #2, I think it makes sense - considering there may be large number of > >snapshots in the cluster. > > > >For #3, can you tell us the use case for the new API ? > > > >Cheers > > > >On Fri, Nov 6, 2015 at 10:12 PM, 王勇强 <[email protected]> wrote: > > > >> the version is hbase0.98.10, find some improvements on snapshot: > >> 1. in class SnapshotManager, line 725, we make a clone of > >> HTableDescriptor, the clone is not so need to make > >> 2 class HBaseAdmin, method > >> public void restoreSnapshot(final String snapshotName, boolean > >> takeFailSafeSnapshot) > >> { > >> TableName tableName = null; > >> for (SnapshotDescription snapshotInfo: listSnapshots()) { > >> if (snapshotInfo.getName().equals(snapshotName)) { > >> tableName = TableName.valueOf(snapshotInfo.getTable()); > >> break; > >> } > >> } > >> ...... > >> } > >> must get all snapshot from master and then get the one we need, can > >> improve this, master just return the snapshot client needed > >> > >> > >> 3. there should better has a method 'isSnapShotExists' in HBaseAdmin > >> > >> > >> > >> > >> > >> > >> > >> >
