I'd be fine if you wanted to file a JIRA and post a patch for #4. Would certainly be cleaner. However, as Ted said, I don't know if its going to be any faster or better, otherwise.
On Sat, Nov 7, 2015 at 10:55 AM Ted Yu <[email protected]> wrote: > 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 > > >> > > >> > > >> > > >> > > >> > > >> > > >> > > >> > > >
