hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hsieh (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HBASE-8352) Rename '.snapshot' directory
Date Tue, 16 Apr 2013 20:19:16 GMT

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

Jonathan Hsieh edited comment on HBASE-8352 at 4/16/13 8:17 PM:
----------------------------------------------------------------

nit: Prefer keeping the no argument version -- this extra generally never used arg makes this
harder to read.
{code}
-      List<SnapshotDescription> snapshots = snapshotManager.getCompletedSnapshots();
+      List<SnapshotDescription> snapshots = snapshotManager.getCompletedSnapshots(null);
{code}


nit: prefer if you create a private more generic version with the path argument but keep the
older no argument version public.  the current way exposes possibilities that don't need to
be.

So instead of this:
{code}
   /**
    * Gets the list of all completed snapshots.
+   * @param snapshotDir snapshot directory
    * @return list of SnapshotDescriptions
    * @throws IOException File system exception
    */
-  public List<SnapshotDescription> getCompletedSnapshots() throws IOException {
+  public List<SnapshotDescription> getCompletedSnapshots(Path snapshotDir) throws IOException
{
     List<SnapshotDescription> snapshotDescs = new ArrayList<SnapshotDescription>();
     // first create the snapshot root path and check to see if it exists
-    Path snapshotDir = SnapshotDescriptionUtils.getSnapshotsDir(rootDir);
     FileSystem fs = master.getMasterFileSystem().getFileSystem();
+    if (snapshotDir == null) snapshotDir = SnapshotDescriptionUtils.getSnapshotsDir(rootDir);
 
     // if there are no snapshots, return an empty list
     if (!fs.exists(snapshotDir)) {
@@ -877,6 +878,15 @@
{code}

have something more like this:
{code}
  public List<SnapshotDescription> getCompletedSnapshots() throws IOException { 
    return getCompletedSnapshots(SnapshotDescriptionUtils.getSnapshotsDir(rootDir));
  }
  private List<SnapshotDescription> getCompletedSnapshots(Path snapshotDir) throws IOException
{
    ...
  }
{code}

Otherwise, lgtm.
                
      was (Author: jmhsieh):
    nit: Prefer keeping the no argument version -- this extra generally never used arg makes
this harder to read.
{code}
-      List<SnapshotDescription> snapshots = snapshotManager.getCompletedSnapshots();
+      List<SnapshotDescription> snapshots = snapshotManager.getCompletedSnapshots(null);
{code}


nit: prefer if you create a private more generic version with the path argument but keep the
older no argument version public.  the current way exposes possibilities that don't need to
be.

So instead of this:
{code}
   /**
    * Gets the list of all completed snapshots.
+   * @param snapshotDir snapshot directory
    * @return list of SnapshotDescriptions
    * @throws IOException File system exception
    */
-  public List<SnapshotDescription> getCompletedSnapshots() throws IOException {
+  public List<SnapshotDescription> getCompletedSnapshots(Path snapshotDir) throws IOException
{
     List<SnapshotDescription> snapshotDescs = new ArrayList<SnapshotDescription>();
     // first create the snapshot root path and check to see if it exists
-    Path snapshotDir = SnapshotDescriptionUtils.getSnapshotsDir(rootDir);
     FileSystem fs = master.getMasterFileSystem().getFileSystem();
+    if (snapshotDir == null) snapshotDir = SnapshotDescriptionUtils.getSnapshotsDir(rootDir);
 
     // if there are no snapshots, return an empty list
     if (!fs.exists(snapshotDir)) {
@@ -877,6 +878,15 @@
{code}

have something more like this:
{code}
  public List<SnapshotDescription> getCompletedSnapshots() throws IOException { 
    getCompletedSnapshots(SnapshotDescriptionUtils.getSnapshotsDir(rootDir));
  }
  private List<SnapshotDescription> getCompletedSnapshots(Path snapshotDir) throws IOException
{
    ...
  }
{code}

Otherwise, lgtm.
                  
> Rename '.snapshot' directory
> ----------------------------
>
>                 Key: HBASE-8352
>                 URL: https://issues.apache.org/jira/browse/HBASE-8352
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Ted Yu
>            Assignee: Ted Yu
>            Priority: Blocker
>             Fix For: 0.98.0, 0.94.7, 0.95.1
>
>         Attachments: 8352-0.94-v1.txt, 8352-0.94-v2.txt, 8352-trunk.txt, 8352-trunk-v2.txt,
8352-trunk-v3.txt, 8352-trunk-v4.txt
>
>
> Testing HBase Snapshot on top of Hadoop's Snapshot branch (http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/),
we found that both features used '.snapshot' directory to store metadata.
> HDFS (built from HDFS-2802 branch) doesn't allow paths with .snapshot as a component
> From discussion on dev@hbase.apache.org, (see http://search-hadoop.com/m/kY6C3cXMs51),
consensus was to rename '.snapshot' directory in HBase so that both features can co-exist
smoothly.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message