[ https://issues.apache.org/jira/browse/HBASE-21098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16598105#comment-16598105 ]
Mingliang Liu commented on HBASE-21098: --------------------------------------- Thanks [~mtylr] for updating the patch. Now it looks better to me overall. We need a clean QA. I expect some checkstyle warnings (e.g. {{getHBaseAdmin()}} -> {{getAdmin()}}). Major comments: # {{TestSnapshotDFSTemporaryDirectory}} is flaky. It attempts to set the {{SNAPSHOT_WORKING_DIR}} as HDFS directory, however it actually is not testing with HDFS directory on the mini DFS cluster: #- does not set a value that is within the default working directory #- the temporary value it sets is *before* the start of mini DFS cluster, so its value will be default local directory. # {{hbase-default.xml}} is the list of configurable properties, serving both as default values and documentation. This patch adds a new config that, if set, will change the default working directory for snapshot. So I think we should put the config (key/description) in that file. As to the default value, it should be empty. Minor comments: # In {{TestExportSnapshotWithTemporaryDirectory.java}}, {{TEST_UTIL.startMiniCluster(1, 3)}} can be {{TEST_UTIL.startMiniCluster(3)}} per HBASE-21071 # Can {{TestExportSnapshotWithTemporaryDirectory::setUpBaseConf()}} call {{TestExportSnapshot::setUpBaseConf()}} first? That will make the subclass setup method two lines of code. # In {{TestExportSnapshotWithTemporaryDirectory}} the temporary directory {{TEMP_DIR}} is under current workspace dir and not cleaned after test. Maybe {{Files.createTempDirectory}} is a better solution. # {{TakeSnapshotHandler::process()}} has another FileSystem exists&delete pattern, we can delete the exists as well. {code:java} if (workingDirFs.exists(workingDir) && !this.workingDirFs.delete(workingDir, true)) { {code} # nit: in method usually we don't need {{this.}} to refer to a private field. It makes more sense to constructors or getters/setters. Discussion: # {{TakeSnapshotHandler::completeSnapshot()}} tries to {{rename}} if it's the same file system, otherwise if different file system or rename fails, it falls back to {{copy}}. The logic might be clearer if we add some comment, and/or split the if clause. I understand it's currently concise so it's up to you. Meanwhile, it may fallback to {{copy}} even if {{rename}} is possible. The reason is that it compares the {{FileSystem}} object using {{equals}}, while {{FileSystem}} does not override the behavior. So by default, it compares the same instance reference. This is indeterministic as {{FileSystem}} can have configurable caching (enabled/disabled) behavior in which case {{FileSystem.get(new URI("myfs://a"), conf)}} _may_ or _may not_ be the same instance as another {{FileSystem.get(new URI("myfs://a"), conf)}}. I assume this is not a big problem though as the default fs cache is enabled. # Just curious, do you have any test system running with this patch where snapshot is on HDFS and rootdir S3? > Improve Snapshot Performance with Temporary Snapshot Directory when rootDir > on S3 > --------------------------------------------------------------------------------- > > Key: HBASE-21098 > URL: https://issues.apache.org/jira/browse/HBASE-21098 > Project: HBase > Issue Type: Improvement > Affects Versions: 3.0.0, 2.1.1 > Reporter: Tyler Mi > Priority: Major > Attachments: HBASE-21098.master.001.patch, > HBASE-21098.master.002.patch, HBASE-21098.master.003.patch, > HBASE-21098.master.004.patch, HBASE-21098.master.005.patch, > HBASE-21098.master.006.patch, HBASE-21098.master.007.patch, > HBASE-21098.master.008.patch > > > When using Apache HBase, the snapshot feature can be used to make a point in > time recovery. To do this, HBase creates a manifest of all the files in all > of the Regions so that those files can be referenced again when a user > restores a snapshot. With HBase's S3 storage mode, developers can store their > data off-cluster on Amazon S3. However, utilizing S3 as a file system is > inefficient in some operations, namely renames. Most Hadoop ecosystem > applications use an atomic rename as a method of committing data. However, > with S3, a rename is a separate copy and then a delete of every file which is > no longer atomic and, in fact, quite costly. In addition, puts and deletes on > S3 have latency issues that traditional filesystems do not encounter when > manipulating the region snapshots to consolidate into a single manifest. When > HBase on S3 users have a significant amount of regions, puts, deletes, and > renames (the final commit stage of the snapshot) become the bottleneck > causing snapshots to take many minutes or even hours to complete. > The purpose of this patch is to increase the overall performance of snapshots > while utilizing HBase on S3 through the use of a temporary directory for the > snapshots that exists on a traditional filesystem like HDFS to circumvent the > bottlenecks. -- This message was sent by Atlassian JIRA (v7.6.3#76005)