Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-10285 Storage Policy Satisfier in HDFS
  3. HDFS-13097

[SPS]: Fix the branch review comments(Part1)

    XMLWordPrintableJSON

    Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: HDFS-10285
    • Fix Version/s: HDFS-10285, 3.2.0
    • Component/s: namenode
    • Labels:
      None

      Description

      Fix the branch review comment. Please refer HDFS-10285 to see more detailed discussion.

      Comment-1)

      BlockManager
      Shouldn’t spsMode be volatile? Although I question why it’s here.

      [Rakesh's reply] Agreed, will do the changes.

      Comment-2)

      Adding SPS methods to this class implies an unexpected coupling of the SPS service to the block manager. Please move them out to prove it’s not tightly coupled.

      [Rakesh's reply] Agreed. I'm planning to create StoragePolicySatisfyManager and keep all the related apis over there.

      Comment-5)

      DatanodeDescriptor
      Why use a synchronized linked list to offer/poll instead of BlockingQueue?

      [Rakesh's reply] Agreed, will do the changes.

      Comment-8)

      DFSUtil
      DFSUtil.removeOverlapBetweenStorageTypes and {{DFSUtil.getSPSWorkMultiplier
      }}. These aren’t generally useful methods so why are they in DFSUtil? Why aren’t they in the only calling class StoragePolicySatisfier?

      [Rakesh's reply] Agreed, Will do the changes.

      Comment-11)

      HdfsServerConstants
      The xattr is called user.hdfs.sps.xattr. Why does the xattr name actually contain the word “xattr”?

      [Rakesh's reply] Sure, will remove “xattr” word.

      Comment-12)

      NameNode
      Super trivial but using the plural pronoun “we” in this exception message is odd. Changing the value isn’t a joint activity.

      For enabling or disabling storage policy satisfier, we must pass either none/internal/external string value only

      [Rakesh's reply] oops, sorry for the mistake. Will change it.

      Comment-15)

      FSDirSatisfyStoragePolicyOp
      satisfyStoragePolicy errors if the xattr is already present. Should this be a no-op? A client re-requesting a storage policy correction probably shouldn't fail.

      unprotectedSatisfyStoragePolicy is called prior to xattr updates, which calls addSPSPathId. To avoid race conditions or inconsistent state if the xattr fails, should call addSPSPathId after xattrs are successfully updated.

      inodeHasSatisfyXAttr calls getXAttrFeature then immediately shorts out if the inode isn't a file. Should do file check first to avoid unnecessary computation.

      In general, not fond of unnecessarily guava. Instead of newArrayListWithCapacity + add, standard Arrays.asList(item) is more succinct.

      [Rakesh'r reply] Agreed, will do the changes.

      Comment-16)

      FSDirStatAndListOp
      Not sure why javadoc was changed to add needLocation. It's already present and now doubled up.

      [Rakesh'r reply] Agreed, will correct it.

       
      Comment-18)

      DFS_MOVER_MOVERTHREADS_DEFAULT is 1000 per DN? If the DN is concurrently doing 1000 moves, it's not in a good state, disk io is probably saturated, and this will only make it much worse. 10 is probably more than sufficient.

      [Rakesh'r reply] Agreed, will make it to smaller value 10.
       
      Comment-22)

      StoragePolicySatisifier
      Should handleException use a double-checked lock to avoid synchronization? Unexpected exceptions should be a rarity, right?
      Speaking of which, it’s not safe to ignore all Throwable in the run loop! You have no idea if data structures are in a sane or consistent state.

      [Rakesh'r reply] Agreed, will do the changes.

        Attachments

        1. HDFS-13097-HDFS-10285.01.patch
          51 kB
          Surendra Singh Lilhore
        2. HDFS-13097-HDFS-10285.02.patch
          78 kB
          Surendra Singh Lilhore
        3. HDFS-13097-HDFS-10285.03.patch
          90 kB
          Rakesh Radhakrishnan
        4. HDFS-13097-HDFS-10285.04.patch
          94 kB
          Rakesh Radhakrishnan

          Activity

            People

            • Assignee:
              surendrasingh Surendra Singh Lilhore
              Reporter:
              surendrasingh Surendra Singh Lilhore
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: