From 3b864842c7058d0bb4da92fa73dade937ceb832d Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Tue, 19 Aug 2014 09:38:33 -0700 Subject: [PATCH] HBASE-11774 Avoid allocating unnecessary tag iterators --- .../security/access/AccessControlLists.java | 4 ++ .../security/access/AccessController.java | 40 +++++++----- .../security/access/TableAuthManager.java | 11 ++-- .../visibility/VisibilityController.java | 26 ++++---- .../visibility/VisibilityLabelFilter.java | 63 ++++++++++--------- .../security/visibility/VisibilityUtils.java | 11 +++- 6 files changed, 93 insertions(+), 62 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java index c00be7b8e61..19e51365f75 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java @@ -686,6 +686,10 @@ public class AccessControlLists { public static List getCellPermissionsForUser(User user, Cell cell) throws IOException { + // Save an object allocation where we can + if (cell.getTagsLength() == 0) { + return null; + } List results = Lists.newArrayList(); Iterator tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index a0dd4384b00..53d3f354c95 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -758,11 +758,14 @@ public class AccessController extends BaseMasterAndRegionObserver for (Map.Entry> e: familyMap.entrySet()) { List newCells = Lists.newArrayList(); for (Cell cell: e.getValue()) { + // Prepend the supplied perms in a new ACL tag to an update list of tags for the cell List tags = Lists.newArrayList(new Tag(AccessControlLists.ACL_TAG_TYPE, perms)); - Iterator tagIterator = CellUtil.tagsIterator(cell.getTagsArray(), + if (cell.getTagsLength() > 0) { + Iterator tagIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); - while (tagIterator.hasNext()) { - tags.add(tagIterator.next()); + while (tagIterator.hasNext()) { + tags.add(tagIterator.next()); + } } // Ensure KeyValue so we can do a scatter gather copy. This is only a win if the // incoming cell type is actually KeyValue. @@ -1694,21 +1697,26 @@ public class AccessController extends BaseMasterAndRegionObserver List tags = Lists.newArrayList(); ListMultimap perms = ArrayListMultimap.create(); if (oldCell != null) { - Iterator tagIterator = CellUtil.tagsIterator(oldCell.getTagsArray(), + // Save an object allocation where we can + if (oldCell.getTagsLength() > 0) { + Iterator tagIterator = CellUtil.tagsIterator(oldCell.getTagsArray(), oldCell.getTagsOffset(), oldCell.getTagsLength()); - while (tagIterator.hasNext()) { - Tag tag = tagIterator.next(); - if (tag.getType() != AccessControlLists.ACL_TAG_TYPE) { - if (LOG.isTraceEnabled()) { - LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType() + - " length " + tag.getTagLength()); + while (tagIterator.hasNext()) { + Tag tag = tagIterator.next(); + if (tag.getType() != AccessControlLists.ACL_TAG_TYPE) { + // Not an ACL tag, just carry it through + if (LOG.isTraceEnabled()) { + LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType() + + " length " + tag.getTagLength()); + } + tags.add(tag); + } else { + // Merge the perms from the older ACL into the current permission set + ListMultimap kvPerms = ProtobufUtil.toUsersAndPermissions( + AccessControlProtos.UsersAndPermissions.newBuilder().mergeFrom( + tag.getBuffer(), tag.getTagOffset(), tag.getTagLength()).build()); + perms.putAll(kvPerms); } - tags.add(tag); - } else { - ListMultimap kvPerms = ProtobufUtil.toUsersAndPermissions( - AccessControlProtos.UsersAndPermissions.newBuilder().mergeFrom( - tag.getBuffer(), tag.getTagOffset(), tag.getTagLength()).build()); - perms.putAll(kvPerms); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java index 8b0bb69177a..cbea05c97bc 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java @@ -356,11 +356,14 @@ public class TableAuthManager { try { List perms = AccessControlLists.getCellPermissionsForUser(user, cell); if (LOG.isTraceEnabled()) { - LOG.trace("Perms for user " + user.getShortName() + " in cell " + cell + ": " + perms); + LOG.trace("Perms for user " + user.getShortName() + " in cell " + cell + ": " + + (perms != null ? perms : "")); } - for (Permission p: perms) { - if (p.implies(action)) { - return true; + if (perms != null) { + for (Permission p: perms) { + if (p.implies(action)) { + return true; + } } } } catch (IOException e) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java index de376902f56..fb62c2bd0e5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java @@ -925,22 +925,26 @@ public class VisibilityController extends BaseMasterAndRegionObserver implements if (checkAuths && user != null && user.getShortName() != null) { auths = this.visibilityManager.getAuthsAsOrdinals(user.getShortName()); } - // Adding all other tags - Iterator tagsItr = CellUtil.tagsIterator(newCell.getTagsArray(), newCell.getTagsOffset(), - newCell.getTagsLength()); - while (tagsItr.hasNext()) { - Tag tag = tagsItr.next(); - if (tag.getType() != VisibilityUtils.VISIBILITY_TAG_TYPE - && tag.getType() != VisibilityUtils.VISIBILITY_EXP_SERIALIZATION_TAG_TYPE) { - tags.add(tag); - } - } + // Prepend new visibility tags to a new list of tags for the cell try { tags.addAll(createVisibilityTags(cellVisibility.getExpression(), true, auths, - user.getShortName())); + user.getShortName())); } catch (ParseException e) { throw new IOException(e); } + // Save an object allocation where we can + if (newCell.getTagsLength() > 0) { + // Carry forward all other tags + Iterator tagsItr = CellUtil.tagsIterator(newCell.getTagsArray(), newCell.getTagsOffset(), + newCell.getTagsLength()); + while (tagsItr.hasNext()) { + Tag tag = tagsItr.next(); + if (tag.getType() != VisibilityUtils.VISIBILITY_TAG_TYPE + && tag.getType() != VisibilityUtils.VISIBILITY_EXP_SERIALIZATION_TAG_TYPE) { + tags.add(tag); + } + } + } // We need to create another KV, unfortunately, because the current new KV // has no space for tags diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelFilter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelFilter.java index 14d69ede066..ea579ff40fe 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelFilter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelFilter.java @@ -80,39 +80,42 @@ class VisibilityLabelFilter extends FilterBase { return ReturnCode.SKIP; } - Iterator tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), - cell.getTagsLength()); boolean visibilityTagPresent = false; - while (tagsItr.hasNext()) { - boolean includeKV = true; - Tag tag = tagsItr.next(); - if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) { - visibilityTagPresent = true; - int offset = tag.getTagOffset(); - int endOffset = offset + tag.getTagLength(); - while (offset < endOffset) { - Pair result = StreamUtils.readRawVarint32(tag.getBuffer(), offset); - int currLabelOrdinal = result.getFirst(); - if (currLabelOrdinal < 0) { - // check for the absence of this label in the Scan Auth labels - // ie. to check BitSet corresponding bit is 0 - int temp = -currLabelOrdinal; - if (this.authLabels.get(temp)) { - includeKV = false; - break; - } - } else { - if (!this.authLabels.get(currLabelOrdinal)) { - includeKV = false; - break; + // Save an object allocation where we can + if (cell.getTagsLength() > 0) { + Iterator tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), + cell.getTagsLength()); + while (tagsItr.hasNext()) { + boolean includeKV = true; + Tag tag = tagsItr.next(); + if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) { + visibilityTagPresent = true; + int offset = tag.getTagOffset(); + int endOffset = offset + tag.getTagLength(); + while (offset < endOffset) { + Pair result = StreamUtils.readRawVarint32(tag.getBuffer(), offset); + int currLabelOrdinal = result.getFirst(); + if (currLabelOrdinal < 0) { + // check for the absence of this label in the Scan Auth labels + // ie. to check BitSet corresponding bit is 0 + int temp = -currLabelOrdinal; + if (this.authLabels.get(temp)) { + includeKV = false; + break; + } + } else { + if (!this.authLabels.get(currLabelOrdinal)) { + includeKV = false; + break; + } } + offset += result.getSecond(); + } + if (includeKV) { + // We got one visibility expression getting evaluated to true. Good to include this KV in + // the result then. + return ReturnCode.INCLUDE; } - offset += result.getSecond(); - } - if (includeKV) { - // We got one visibility expression getting evaluated to true. Good to include this KV in - // the result then. - return ReturnCode.INCLUDE; } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityUtils.java index 35843ff90cf..f77eb96ab64 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityUtils.java @@ -176,6 +176,9 @@ public class VisibilityUtils { */ public static boolean getVisibilityTags(Cell cell, List tags) { boolean sortedOrder = false; + if (cell.getTagsLength() == 0) { + return sortedOrder; + } Iterator tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); while (tagsIterator.hasNext()) { @@ -200,6 +203,9 @@ public class VisibilityUtils { * @return true if found, false if not found */ public static boolean isVisibilityTagsPresent(Cell cell) { + if (cell.getTagsLength() == 0) { + return false; + } Iterator tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); while (tagsIterator.hasNext()) { @@ -270,9 +276,12 @@ public class VisibilityUtils { } private static List> sortTagsBasedOnOrdinal(Cell cell) throws IOException { + List> fullTagsList = new ArrayList>(); + if (cell.getTagsLength() == 0) { + return fullTagsList; + } Iterator tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); - List> fullTagsList = new ArrayList>(); while (tagsItr.hasNext()) { Tag tag = tagsItr.next(); if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) {