HDFS-1081. Performance regression in DistributedFileSystem::getFileBlockLocations in secure systems.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/hdfs/trunk@966016 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -87,6 +87,9 @@ Trunk (unreleased changes)
 
     HDFS-1140. Speedup INode.getPathComponents. (Dmytro Molkov via shv)
 
+    HDFS-1081. Performance regression in 
+    DistributedFileSystem::getFileBlockLocations in secure systems (jghoman)
+
   BUG FIXES
 
     HDFS-1039. Adding test for  JspHelper.getUGI(jnp via boryas)

src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java

@@ -68,9 +68,9 @@ public interface ClientProtocol extends VersionedProtocol {
    * Compared to the previous version the following changes have been introduced:
    * (Only the latest change is reflected.
    * The log of historical changes can be retrieved from the svn).
-   * 60: Replace full getListing with iterative getListinng.
+   * 61: HDFS-1081. Performance optimization on getBlocksLocation().
    */
-  public static final long versionID = 60L;
+  public static final long versionID = 61L;
   
   ///////////////////////////////////////
   // File contents

src/java/org/apache/hadoop/hdfs/security/token/block/BlockTokenIdentifier.java

@@ -21,6 +21,7 @@ package org.apache.hadoop.hdfs.security.token.block;
 import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.EnumSet;
 
 import org.apache.hadoop.classification.InterfaceAudience;
@@ -37,17 +38,23 @@ public class BlockTokenIdentifier extends TokenIdentifier {
   private long expiryDate;
   private int keyId;
   private String userId;
-  private long blockId;
+  private long [] blockIds;
   private EnumSet<AccessMode> modes;
 
+  private byte [] cache;
+  
   public BlockTokenIdentifier() {
-    this(null, 0, EnumSet.noneOf(AccessMode.class));
+    this(null, new long [] {}, EnumSet.noneOf(AccessMode.class));
   }
 
-  public BlockTokenIdentifier(String userId, long blockId,
+  public BlockTokenIdentifier(String userId, long [] blockIds,
       EnumSet<AccessMode> modes) {
+    if(blockIds == null) 
+      throw new IllegalArgumentException("blockIds can't be null");
+    this.cache = null;
     this.userId = userId;
-    this.blockId = blockId;
+    this.blockIds = Arrays.copyOf(blockIds, blockIds.length);
+    Arrays.sort(this.blockIds);
     this.modes = modes == null ? EnumSet.noneOf(AccessMode.class) : modes;
   }
 
@@ -59,7 +66,7 @@ public class BlockTokenIdentifier extends TokenIdentifier {
   @Override
   public UserGroupInformation getUser() {
     if (userId == null || "".equals(userId)) {
-      return UserGroupInformation.createRemoteUser(Long.toString(blockId));
+      return UserGroupInformation.createRemoteUser(Arrays.toString(blockIds));
     }
     return UserGroupInformation.createRemoteUser(userId);
   }
@@ -69,6 +76,7 @@ public class BlockTokenIdentifier extends TokenIdentifier {
   }
 
   public void setExpiryDate(long expiryDate) {
+    this.cache = null;
     this.expiryDate = expiryDate;
   }
 
@@ -77,6 +85,7 @@ public class BlockTokenIdentifier extends TokenIdentifier {
   }
 
   public void setKeyId(int keyId) {
+    this.cache = null;
     this.keyId = keyId;
   }
 
@@ -84,18 +93,36 @@ public class BlockTokenIdentifier extends TokenIdentifier {
     return userId;
   }
 
-  public long getBlockId() {
-    return blockId;
+  /**
+   * Return sorted array of blockIds this {@link BlockTokenIdentifier} includes
+   */
+  public long [] getBlockIds() {
+    return blockIds;
+  }
+  
+  /**
+   * Is specified blockId included in this BlockTokenIdentifier?
+   */
+  public boolean isBlockIncluded(long blockId) {
+    switch(blockIds.length) {
+    case 1:
+      return blockIds[0] == blockId;
+    case 2:
+      return (blockIds[0] == blockId) || (blockIds[1] == blockId);
+    default:
+      return Arrays.binarySearch(blockIds, blockId) >= 0;  
+    }
   }
-
+  
   public EnumSet<AccessMode> getAccessModes() {
     return modes;
   }
 
+  @Override
   public String toString() {
     return "block_token_identifier (expiryDate=" + this.getExpiryDate()
         + ", keyId=" + this.getKeyId() + ", userId=" + this.getUserId()
-        + ", blockId=" + this.getBlockId() + ", access modes="
+        + ", blockId=" + Arrays.toString(blockIds) + ", access modes="
         + this.getAccessModes() + ")";
   }
 
@@ -111,7 +138,8 @@ public class BlockTokenIdentifier extends TokenIdentifier {
     if (obj instanceof BlockTokenIdentifier) {
       BlockTokenIdentifier that = (BlockTokenIdentifier) obj;
       return this.expiryDate == that.expiryDate && this.keyId == that.keyId
-          && isEqual(this.userId, that.userId) && this.blockId == that.blockId
+          && isEqual(this.userId, that.userId) 
+          && Arrays.equals(this.blockIds, that.blockIds)
           && isEqual(this.modes, that.modes);
     }
     return false;
@@ -119,15 +147,18 @@ public class BlockTokenIdentifier extends TokenIdentifier {
 
   /** {@inheritDoc} */
   public int hashCode() {
-    return (int) expiryDate ^ keyId ^ (int) blockId ^ modes.hashCode()
+    return (int) expiryDate ^ keyId ^ Arrays.hashCode(blockIds) ^ modes.hashCode()
         ^ (userId == null ? 0 : userId.hashCode());
   }
 
   public void readFields(DataInput in) throws IOException {
+    cache = null;
     expiryDate = WritableUtils.readVLong(in);
     keyId = WritableUtils.readVInt(in);
     userId = WritableUtils.readString(in);
-    blockId = WritableUtils.readVLong(in);
+    blockIds = new long[WritableUtils.readVInt(in)];
+    for(int i = 0; i < blockIds.length; i++)
+      blockIds[i] = WritableUtils.readVLong(in);
     int length = WritableUtils.readVInt(in);
     for (int i = 0; i < length; i++) {
       modes.add(WritableUtils.readEnum(in, AccessMode.class));
@@ -138,10 +169,19 @@ public class BlockTokenIdentifier extends TokenIdentifier {
     WritableUtils.writeVLong(out, expiryDate);
     WritableUtils.writeVInt(out, keyId);
     WritableUtils.writeString(out, userId);
-    WritableUtils.writeVLong(out, blockId);
+    WritableUtils.writeVInt(out, blockIds.length);
+    for(int i = 0; i < blockIds.length; i++)
+      WritableUtils.writeVLong(out, blockIds[i]);
     WritableUtils.writeVInt(out, modes.size());
     for (AccessMode aMode : modes) {
       WritableUtils.writeEnum(out, aMode);
     }
   }
+  
+  @Override
+  public byte[] getBytes() {
+    if(cache == null) cache = super.getBytes();
+    
+    return cache;
+  }
 }

src/java/org/apache/hadoop/hdfs/security/token/block/BlockTokenSecretManager.java

@@ -176,16 +176,29 @@ public class BlockTokenSecretManager extends
   /** Generate an block token for current user */
   public Token<BlockTokenIdentifier> generateToken(Block block,
       EnumSet<AccessMode> modes) throws IOException {
-    UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
-    String userID = (ugi == null ? null : ugi.getShortUserName());
-    return generateToken(userID, block, modes);
+    return generateToken(new long [] { block.getBlockId() }, modes);
   }
 
   /** Generate a block token for a specified user */
   public Token<BlockTokenIdentifier> generateToken(String userId, Block block,
       EnumSet<AccessMode> modes) throws IOException {
-    BlockTokenIdentifier id = new BlockTokenIdentifier(userId, block
-        .getBlockId(), modes);
+    return generateToken(userId, new long [] { block.getBlockId() }, modes);
+  }
+
+  /** Generate a block token for the current user based on a collection
+   * of blockIds
+   */
+  public Token<BlockTokenIdentifier> generateToken(long[] blockIds,
+      EnumSet<AccessMode> modes) throws IOException {
+    UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
+    String userID = (ugi == null ? null : ugi.getShortUserName());
+    return generateToken(userID, blockIds, modes);
+  }
+  
+  /** Generate a block token based on a collection of blockIds */
+  public Token<BlockTokenIdentifier> generateToken(String userID,
+      long[] blockIds, EnumSet<AccessMode> modes) {
+    BlockTokenIdentifier id = new BlockTokenIdentifier(userID, blockIds, modes);
     return new Token<BlockTokenIdentifier>(id, this);
   }
 
@@ -204,7 +217,7 @@ public class BlockTokenSecretManager extends
       throw new InvalidToken("Block token with " + id.toString()
           + " doesn't belong to user " + userId);
     }
-    if (id.getBlockId() != block.getBlockId()) {
+    if (!id.isBlockIncluded(block.getBlockId())) {
       throw new InvalidToken("Block token with " + id.toString()
           + " doesn't apply to block " + block);
     }

src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

@@ -24,6 +24,7 @@ import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.conf.*;
 import org.apache.hadoop.hdfs.DFSUtil;
 import org.apache.hadoop.hdfs.protocol.*;
+import org.apache.hadoop.hdfs.security.token.block.BlockTokenIdentifier;
 import org.apache.hadoop.hdfs.security.token.block.BlockTokenSecretManager;
 import org.apache.hadoop.hdfs.security.token.block.ExportedBlockKeys;
 import org.apache.hadoop.hdfs.server.common.GenerationStamp;
@@ -792,26 +793,36 @@ public class FSNamesystem implements FSConstants, FSNamesystemMBean, FSClusterSt
         LOG.debug("last = " + last);
       }
 
-      if (!last.isComplete()) {
-        return new LocatedBlocks(n, inode.isUnderConstruction(), locatedblocks,
-            blockManager.getBlockLocation(last, n), false);
-      }
-      else {
+      if(isBlockTokenEnabled) setBlockTokens(locatedblocks);
+
+      if (last.isComplete()) {
         return new LocatedBlocks(n, inode.isUnderConstruction(), locatedblocks,
             blockManager.getBlockLocation(last, n-last.getNumBytes()), true);
+      } else {
+        return new LocatedBlocks(n, inode.isUnderConstruction(), locatedblocks,
+            blockManager.getBlockLocation(last, n), false);
       }
     }
   }
 
+  /** Generate a combined block token for all the blocks to be returned. */
+  private void setBlockTokens(List<LocatedBlock> locatedBlocks) throws IOException {
+    long [] blockIds = new long[locatedBlocks.size()];
+    for(int i = 0; i < blockIds.length; i++) {
+      blockIds[i] = locatedBlocks.get(i).getBlock().getBlockId();
+    }
+    
+    Token<BlockTokenIdentifier> token = 
+      blockTokenSecretManager.generateToken(blockIds, 
+          EnumSet.of(BlockTokenSecretManager.AccessMode.READ));
+    
+    for(LocatedBlock l : locatedBlocks) l.setBlockToken(token);
+  }
+
   /** Create a LocatedBlock. */
   LocatedBlock createLocatedBlock(final Block b, final DatanodeInfo[] locations,
       final long offset, final boolean corrupt) throws IOException {
-    final LocatedBlock lb = new LocatedBlock(b, locations, offset, corrupt);
-    if (isBlockTokenEnabled) {
-      lb.setBlockToken(blockTokenSecretManager.generateToken(b,
-          EnumSet.of(BlockTokenSecretManager.AccessMode.READ)));
-    }
-    return lb;
+    return new LocatedBlock(b, locations, offset, corrupt);
   }
   
   /**

src/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java

@@ -44,9 +44,9 @@ import org.apache.avro.reflect.Nullable;
 @InterfaceAudience.Private
 public interface DatanodeProtocol extends VersionedProtocol {
   /**
-   * 24: register() renamed registerDatanode()
+   * 25: HDFS-1081. Performance optimization on getBlocksLocation()
    */
-  public static final long versionID = 24L;
+  public static final long versionID = 25L;
   
   // error code
   final static int NOTIFY = 0;

src/test/hdfs/org/apache/hadoop/hdfs/security/token/block/TestBlockToken.java

@@ -22,6 +22,7 @@ import java.io.ByteArrayInputStream;
 import java.io.DataInputStream;
 import java.io.IOException;
 import java.net.InetSocketAddress;
+import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.Set;
 
@@ -102,15 +103,16 @@ public class TestBlockToken {
       Set<TokenIdentifier> tokenIds = UserGroupInformation.getCurrentUser()
           .getTokenIdentifiers();
       assertEquals("Only one BlockTokenIdentifier expected", 1, tokenIds.size());
-      long result = 0;
+      long [] result = {0};
       for (TokenIdentifier tokenId : tokenIds) {
         BlockTokenIdentifier id = (BlockTokenIdentifier) tokenId;
         LOG.info("Got: " + id.toString());
         assertTrue("Received BlockTokenIdentifier is wrong", ident.equals(id));
         sm.checkAccess(id, null, block, BlockTokenSecretManager.AccessMode.WRITE);
-        result = id.getBlockId();
+        result = id.getBlockIds();
       }
-      return result;
+      assertEquals("Got more than one block back", 1, result.length);
+      return result[0];
     }
   }
 
@@ -221,4 +223,28 @@ public class TestBlockToken {
     }
   }
 
+  @Test
+  public void collectionOfBlocksActsSanely() {
+    final long[][] testBlockIds = new long [][] {{99l, 7l, -32l, 0l},
+                                                 {},
+                                                 {42l},
+                                                 {-5235l, 2352}};
+    final long [] notBlockIds = new long [] { 32l, 1l, -23423423l};
+
+    for(long [] bids : testBlockIds) {
+      BlockTokenIdentifier bti = new BlockTokenIdentifier("Madame Butterfly", 
+          bids, EnumSet.noneOf(BlockTokenSecretManager.AccessMode.class));
+
+      for(long bid : bids) assertTrue(bti.isBlockIncluded(bid));
+
+      for(long nbid : notBlockIds) assertFalse(bti.isBlockIncluded(nbid));
+
+      // BlockTokenIdentifiers maintain a sorted array of the block Ids.
+      long[] sorted = Arrays.copyOf(bids, bids.length);
+      Arrays.sort(sorted);
+
+      assertTrue(Arrays.toString(bids)+" doesn't equal "+Arrays.toString(sorted), 
+          Arrays.equals(bti.getBlockIds(), sorted));
+    }
+  }
 }