[jira] [Comment Edited] (HBASE-21098) Improve Snapshot Performance with Temporary Snapshot Directory when rootDir on S3

["Mingliang Liu (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-21098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16598105#comment-16598105 ] 

Mingliang Liu edited comment on HBASE-21098 at 9/5/18 2:41 AM:
---------------------------------------------------------------

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, as the test:-
 -- -does not set a value that is within the default working directory. Precondition check in {{TakeSnapshotHandler}} should fail if it's not.-
 -- -sets the temporary value *before* the start of mini DFS cluster, so its value will be default local directory, instead of DFS temporary 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}}. Is this the copy after rename fails necessary?
 -- 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?


was (Author: liuml07):
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, as the test:
#- does not set a value that is within the default working directory. Precondition check in {{TakeSnapshotHandler}} should fail if it's not.
#- sets the temporary value *before* the start of mini DFS cluster, so its value will be default local directory, instead of DFS temporary 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}}. Is this the copy after rename fails necessary?
#- 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, 1.4.8, 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, HBASE-21098.master.009.patch, HBASE-21098.master.010.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)