Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2006 ability to support storing extended attributes per file
  3. HDFS-6346

Optimize OP_SET_XATTRS by persisting single Xattr entry per setXattr/removeXattr api call

    Details

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

      Description

      When a new xattrs set on an Inode, it may add this with OP_SET_XATTRS and let's say [user.name1:value1]
      On a next call if we set another xattrs, then it may store along with older existing xattrs again. It may be like [user.name1:value1, user.name2:value2]
      So, on adding more xattrs on same Inode, that list may grow and we keep store the entries of already existing name, value fairs.
      Right now we defaulted the max Xattrs on an Inode to 32 and configured. If user modified it to much larger value and start setting xattrs, then edits loading may create issue like my above example.
      But I didn't refer any usecase of having large number of xattrs, this is just the assumption to consider a case. My biggest doubt is whether we will have such real usecases to have huge xattrs on a single INode.
      So, here is a thought on having OP_SET_XATTR for each setXAttr operation to be logged, When we replay them we need to consolidate. This is some initial thought we can think more if others also feel we need to consider this case to handle.

      Otherwise we endup storing Xattrs entries in editlog file as n(n+1)/2 where n is number xattrs for a file/dir. This may be issue only when we have large number configured max xattrs for inode.

      1. editsStored
        5 kB
        Yi Liu
      2. HDFS-6346.1.patch
        18 kB
        Yi Liu
      3. HDFS-6346.2.patch
        18 kB
        Yi Liu
      4. HDFS-6346.patch
        17 kB
        Yi Liu

        Activity

        Hide
        Yi Liu added a comment -

        Thanks Uma for review.
        Right, we can optimize the op code of edit log. In the new patch, we have OP_SET_XATTR and OP_REMOVE_XATTR.
        For setXAttr, we don't need to log the XAttrSetFlag, since for any flag, edit log happens only after operation successes. When loading edit log, we can specify XAttrSetFlag.CREATE|XAttrSetFlag.REPLACE.

        Show
        Yi Liu added a comment - Thanks Uma for review. Right, we can optimize the op code of edit log. In the new patch, we have OP_SET_XATTR and OP_REMOVE_XATTR. For setXAttr, we don't need to log the XAttrSetFlag, since for any flag, edit log happens only after operation successes. When loading edit log, we can specify XAttrSetFlag.CREATE|XAttrSetFlag.REPLACE.
        Hide
        Uma Maheswara Rao G added a comment -

        Patch looks great!

        I have few initial comments below:

         if (removedXAttr != null) {
                fsImage.getEditLog().logRemoveXAttr(src, removedXAttr);
              }
        

        Do you think, we need warn/log to the user that xattr attempting to remove does not exist?

         XAttr unprotectedRemoveXAttr(String src,
              XAttr xAttr) throws IOException {
        

        Need format

         XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId);
            
            return existingXAttrs.size() == newXAttrs.size() ? null : xAttr;
        

        Pls remove empty line between

        When existing and updated xattrs equal then we need not even call updateINodeXAttr

        so code can be changed from

        
        

        to

         if (existingXAttrs.size() != newXAttrs.size()) {
              XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId);
              return xAttr;
            }
            return null; 

        ?

        I will continue my review tomorrow on this and post if there are any comments.

        Show
        Uma Maheswara Rao G added a comment - Patch looks great! I have few initial comments below: if (removedXAttr != null ) { fsImage.getEditLog().logRemoveXAttr(src, removedXAttr); } Do you think, we need warn/log to the user that xattr attempting to remove does not exist? XAttr unprotectedRemoveXAttr( String src, XAttr xAttr) throws IOException { Need format XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId); return existingXAttrs.size() == newXAttrs.size() ? null : xAttr; Pls remove empty line between When existing and updated xattrs equal then we need not even call updateINodeXAttr so code can be changed from to if (existingXAttrs.size() != newXAttrs.size()) { XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId); return xAttr; } return null ; ? I will continue my review tomorrow on this and post if there are any comments.
        Hide
        Yi Liu added a comment -

        Thanks Uma for the review.

        Do you think, we need warn/log to the user that xattr attempting to remove does not exist?

        make sense, let's add a log when removing nonexistent xattr.

        Pls remove empty line between

        OK

        When existing and updated xattrs equal then we need not even call updateINodeXAttr. So code can be changed to

        if (existingXAttrs.size() != newXAttrs.size()) {
          XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId);
          return xAttr;
        }
        return null; 
        

        Right, I will update it

        Show
        Yi Liu added a comment - Thanks Uma for the review. Do you think, we need warn/log to the user that xattr attempting to remove does not exist? make sense, let's add a log when removing nonexistent xattr. Pls remove empty line between OK When existing and updated xattrs equal then we need not even call updateINodeXAttr. So code can be changed to if (existingXAttrs.size() != newXAttrs.size()) { XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId); return xAttr; } return null ; Right, I will update it
        Hide
        Uma Maheswara Rao G added a comment - - edited

        Please modify log message something like:

        "Xattr " + xattr + " does not exist on the path " + path

        Other than this patch looks great to me. +1 on addressing this comment.

        I have verified by following operations:
        150 setXattr and 150 removeXattr operations with the key as user.a1<number from 1-150> and value as

        {0x31, 0x32, 0x33}

        Resulted editlog file size with out patch is 343KB and After applying the patch, the editlog size reduced to 11KB.

        Thanks a lot, for taking this and improving.

        Show
        Uma Maheswara Rao G added a comment - - edited Please modify log message something like: "Xattr " + xattr + " does not exist on the path " + path Other than this patch looks great to me. +1 on addressing this comment. I have verified by following operations: 150 setXattr and 150 removeXattr operations with the key as user.a1<number from 1-150> and value as {0x31, 0x32, 0x33} Resulted editlog file size with out patch is 343KB and After applying the patch, the editlog size reduced to 11KB . Thanks a lot, for taking this and improving.
        Hide
        Yi Liu added a comment -

        Uma, thanks for review and collecting the improvement data. That's great. The new patch includes update for your comment.

        Show
        Yi Liu added a comment - Uma, thanks for review and collecting the improvement data. That's great. The new patch includes update for your comment.
        Hide
        Uma Maheswara Rao G added a comment -

        +1 Patch looks good to me.

        Show
        Uma Maheswara Rao G added a comment - +1 Patch looks good to me.
        Hide
        Uma Maheswara Rao G added a comment -

        I have just committed this to branch!

        Show
        Uma Maheswara Rao G added a comment - I have just committed this to branch!

          People

          • Assignee:
            Yi Liu
            Reporter:
            Uma Maheswara Rao G
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development