[
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 8/31/18 1:31 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:
#- 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, 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
>
>
> 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)
