Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Snapshot (HDFS-2802)
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Implement the disallowSnapshot(..) in FSNamesystem and add a resetSnapshottable(..) to SnapshotManager.

      1. h4149_20121104.patch
        18 kB
        Tsz Wo Nicholas Sze
      2. h4149_20121104b.patch
        21 kB
        Tsz Wo Nicholas Sze

        Activity

        Hide
        Tsz Wo Nicholas Sze added a comment -

        h4149_20121104.patch: 1st patch.

        Show
        Tsz Wo Nicholas Sze added a comment - h4149_20121104.patch: 1st patch.
        Hide
        Suresh Srinivas added a comment -

        Comments:

        1. DistributedFileSystem.java - change the param name to createSnapshot from snapshotRoot to root. I suggest making similar change in the FileSystem.java as well.
        2. SnapshotManager#resetSnapshottable - Along with "The directory has snapshot(s)...", please also add, "Please redo the operation after removing all the snapshots."
        3. InodeDirectorySnapshottable
          • Member variable #snapshots - if there is a specific order in which snapshots are stored (that decreasing order creation time), please add it in the javadoc for this member variable.
          • "Number of snapshots is allowed." should be "Number of snapshots allowed."
          • It may be good to add javadoc to the InodeDirectorySnapshottable class to say it is synchronized external by {@link SnapshotManager}
          • Make getNumSnapshots() package private so that only SnapshotManager can use it with proper synchronization?
          • Javadoc for #getSnapshotRoot() to describe what is snapshot root would help. Also change variable name "name" to snapshotName.
        Show
        Suresh Srinivas added a comment - Comments: DistributedFileSystem.java - change the param name to createSnapshot from snapshotRoot to root. I suggest making similar change in the FileSystem.java as well. SnapshotManager#resetSnapshottable - Along with "The directory has snapshot(s)...", please also add, "Please redo the operation after removing all the snapshots." InodeDirectorySnapshottable Member variable #snapshots - if there is a specific order in which snapshots are stored (that decreasing order creation time), please add it in the javadoc for this member variable. "Number of snapshots is allowed." should be "Number of snapshots allowed." It may be good to add javadoc to the InodeDirectorySnapshottable class to say it is synchronized external by {@link SnapshotManager} Make getNumSnapshots() package private so that only SnapshotManager can use it with proper synchronization? Javadoc for #getSnapshotRoot() to describe what is snapshot root would help. Also change variable name "name" to snapshotName.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h4149_20121104b.patch: incorporates the review comments and fixes some bug in dumpTreeRecursively(..).

        Show
        Tsz Wo Nicholas Sze added a comment - h4149_20121104b.patch: incorporates the review comments and fixes some bug in dumpTreeRecursively(..).
        Hide
        Suresh Srinivas added a comment -

        +1 for the patch.

        Show
        Suresh Srinivas added a comment - +1 for the patch.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I have committed this.

        Show
        Tsz Wo Nicholas Sze added a comment - I have committed this.

          People

          • Assignee:
            Tsz Wo Nicholas Sze
            Reporter:
            Tsz Wo Nicholas Sze
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development