From 7cdd02efeb76cc88593a47354d016956f31a1ea3 Mon Sep 17 00:00:00 2001 From: Vinayakumar B Date: Mon, 13 Jan 2020 12:21:18 +0530 Subject: [PATCH 1/4] HADOOP-16621. [pb-upgrade] Remove Protobuf classes from signatures of Public APIs --- .../org/apache/hadoop/ipc/ProtobufHelper.java | 69 +++++++++++++++++++ .../apache/hadoop/security/Credentials.java | 5 +- .../apache/hadoop/security/token/Token.java | 28 -------- .../hdfs/protocolPB/PBHelperClient.java | 42 ++--------- 4 files changed, 78 insertions(+), 66 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java index e30f28a698a43..bfd82cebefd06 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java @@ -18,9 +18,15 @@ package org.apache.hadoop.ipc; import java.io.IOException; +import java.util.concurrent.ConcurrentHashMap; import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.io.Text; +import org.apache.hadoop.security.proto.SecurityProtos; +import org.apache.hadoop.security.token.Token; +import org.apache.hadoop.security.token.TokenIdentifier; +import com.google.protobuf.ByteString; import com.google.protobuf.ServiceException; /** @@ -46,4 +52,67 @@ public static IOException getRemoteException(ServiceException se) { } return e instanceof IOException ? (IOException) e : new IOException(se); } + + + /** + * Map used to cache fixed strings to ByteStrings. Since there is no + * automatic expiration policy, only use this for strings from a fixed, small + * set. + *

+ * This map should not be accessed directly. Used the getFixedByteString + * methods instead. + */ + private static ConcurrentHashMap fixedByteStringCache = + new ConcurrentHashMap<>(); + + /** + * Get the ByteString for frequently used fixed and small set strings. + * @param key string + * @return + */ + public static ByteString getFixedByteString(Text key) { + ByteString value = fixedByteStringCache.get(key); + if (value == null) { + value = ByteString.copyFromUtf8(key.toString()); + fixedByteStringCache.put(new Text(key.copyBytes()), value); + } + return value; + } + + /** + * Get the ByteString for frequently used fixed and small set strings. + * @param key string + * @return + */ + public static ByteString getFixedByteString(String key) { + ByteString value = fixedByteStringCache.get(key); + if (value == null) { + value = ByteString.copyFromUtf8(key); + fixedByteStringCache.put(key, value); + } + return value; + } + + public static ByteString getByteString(byte[] bytes) { + // return singleton to reduce object allocation + return (bytes.length == 0) ? ByteString.EMPTY : ByteString.copyFrom(bytes); + } + + public static Token convert( + SecurityProtos.TokenProto tokenProto) { + Token token = new Token<>( + tokenProto.getIdentifier().toByteArray(), + tokenProto.getPassword().toByteArray(), new Text(tokenProto.getKind()), + new Text(tokenProto.getService())); + return token; + } + + public static SecurityProtos.TokenProto convert(Token tok) { + SecurityProtos.TokenProto.Builder builder = SecurityProtos.TokenProto.newBuilder(). + setIdentifier(getByteString(tok.getIdentifier())). + setPassword(getByteString(tok.getPassword())). + setKindBytes(getFixedByteString(tok.getKind())). + setServiceBytes(getFixedByteString(tok.getService())); + return builder.build(); + } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java index 37cf021d41c51..02a558ebb08aa 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java @@ -46,6 +46,7 @@ import org.apache.hadoop.io.Text; import org.apache.hadoop.io.Writable; import org.apache.hadoop.io.WritableUtils; +import org.apache.hadoop.ipc.ProtobufHelper; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.security.token.TokenIdentifier; import org.apache.hadoop.security.proto.SecurityProtos.CredentialsKVProto; @@ -368,7 +369,7 @@ void writeProto(DataOutput out) throws IOException { CredentialsKVProto.Builder kv = CredentialsKVProto.newBuilder(). setAliasBytes(ByteString.copyFrom( e.getKey().getBytes(), 0, e.getKey().getLength())). - setToken(e.getValue().toTokenProto()); + setToken(ProtobufHelper.convert(e.getValue())); storage.addTokens(kv.build()); } @@ -390,7 +391,7 @@ void readProto(DataInput in) throws IOException { CredentialsProto storage = CredentialsProto.parseDelimitedFrom((DataInputStream)in); for (CredentialsKVProto kv : storage.getTokensList()) { addToken(new Text(kv.getAliasBytes().toByteArray()), - (Token) new Token(kv.getToken())); + ProtobufHelper.convert(kv.getToken())); } for (CredentialsKVProto kv : storage.getSecretsList()) { addSecretKey(new Text(kv.getAliasBytes().toByteArray()), diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java index 487dd4625202e..4f0f6fc4d444a 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/Token.java @@ -19,7 +19,6 @@ package org.apache.hadoop.security.token; import com.google.common.collect.Maps; -import com.google.protobuf.ByteString; import com.google.common.primitives.Bytes; import org.apache.commons.codec.binary.Base64; @@ -28,7 +27,6 @@ import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.io.*; -import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; import org.apache.hadoop.util.ReflectionUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -117,32 +115,6 @@ public Token copyToken() { return new Token(this); } - /** - * Construct a Token from a TokenProto. - * @param tokenPB the TokenProto object - */ - public Token(TokenProto tokenPB) { - this.identifier = tokenPB.getIdentifier().toByteArray(); - this.password = tokenPB.getPassword().toByteArray(); - this.kind = new Text(tokenPB.getKindBytes().toByteArray()); - this.service = new Text(tokenPB.getServiceBytes().toByteArray()); - } - - /** - * Construct a TokenProto from this Token instance. - * @return a new TokenProto object holding copies of data in this instance - */ - public TokenProto toTokenProto() { - return TokenProto.newBuilder(). - setIdentifier(ByteString.copyFrom(this.getIdentifier())). - setPassword(ByteString.copyFrom(this.getPassword())). - setKindBytes(ByteString.copyFrom( - this.getKind().getBytes(), 0, this.getKind().getLength())). - setServiceBytes(ByteString.copyFrom( - this.getService().getBytes(), 0, this.getService().getLength())). - build(); - } - /** * Get the token identifier's byte representation. * @return the token identifier's byte representation diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java index 691ac54ff29ee..fd3098adc9bc5 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import com.google.common.base.Preconditions; import com.google.common.cache.CacheBuilder; @@ -207,6 +206,7 @@ import org.apache.hadoop.io.EnumSetWritable; import org.apache.hadoop.io.Text; import org.apache.hadoop.io.erasurecode.ECSchema; +import org.apache.hadoop.ipc.ProtobufHelper; import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.util.ChunkedArrayList; @@ -232,33 +232,12 @@ public class PBHelperClient { private static final FsAction[] FSACTION_VALUES = FsAction.values(); - /** - * Map used to cache fixed strings to ByteStrings. Since there is no - * automatic expiration policy, only use this for strings from a fixed, small - * set. - *

- * This map should not be accessed directly. Used the getFixedByteString - * methods instead. - */ - private static ConcurrentHashMap fixedByteStringCache = - new ConcurrentHashMap<>(); - private static ByteString getFixedByteString(Text key) { - ByteString value = fixedByteStringCache.get(key); - if (value == null) { - value = ByteString.copyFromUtf8(key.toString()); - fixedByteStringCache.put(new Text(key.copyBytes()), value); - } - return value; + return ProtobufHelper.getFixedByteString(key); } private static ByteString getFixedByteString(String key) { - ByteString value = fixedByteStringCache.get(key); - if (value == null) { - value = ByteString.copyFromUtf8(key); - fixedByteStringCache.put(key, value); - } - return value; + return ProtobufHelper.getFixedByteString(key); } /** @@ -281,7 +260,7 @@ private PBHelperClient() { public static ByteString getByteString(byte[] bytes) { // return singleton to reduce object allocation - return (bytes.length == 0) ? ByteString.EMPTY : ByteString.copyFrom(bytes); + return ProtobufHelper.getByteString(bytes); } public static ShmId convert(ShortCircuitShmIdProto shmId) { @@ -349,12 +328,7 @@ public static ExtendedBlockProto convert(final ExtendedBlock b) { } public static TokenProto convert(Token tok) { - TokenProto.Builder builder = TokenProto.newBuilder(). - setIdentifier(getByteString(tok.getIdentifier())). - setPassword(getByteString(tok.getPassword())). - setKindBytes(getFixedByteString(tok.getKind())). - setServiceBytes(getFixedByteString(tok.getService())); - return builder.build(); + return ProtobufHelper.convert(tok); } public static ShortCircuitShmIdProto convert(ShmId shmId) { @@ -832,11 +806,7 @@ public static StorageType[] convertStorageTypes( public static Token convert( TokenProto blockToken) { - Token token = - new Token<>(blockToken.getIdentifier() - .toByteArray(), blockToken.getPassword().toByteArray(), new Text( - blockToken.getKind()), new Text(blockToken.getService())); - return token; + return (Token) ProtobufHelper.convert(blockToken); } // DatanodeId From e5db4277b8c12e8664405cba47d5b8abcadd3e60 Mon Sep 17 00:00:00 2001 From: Vinayakumar B Date: Mon, 13 Jan 2020 15:31:25 +0530 Subject: [PATCH 2/4] HADOOP-16621. [pb-upgrade] Remove Protobuf classes from signatures of Public APIs. Fixed findbugs --- .../hadoop-common/dev-support/findbugsExcludeFile.xml | 6 ++++++ .../hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml | 5 ----- .../org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java | 4 ---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml b/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml index 802197e33cbcd..cf5c3874d1063 100644 --- a/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml +++ b/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml @@ -460,4 +460,10 @@ + + + + + + diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml b/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml index fa9654b16a037..8e2bc944e87a6 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml +++ b/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml @@ -91,10 +91,5 @@ - - - - - diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java index fd3098adc9bc5..7afa4994241e3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java @@ -232,10 +232,6 @@ public class PBHelperClient { private static final FsAction[] FSACTION_VALUES = FsAction.values(); - private static ByteString getFixedByteString(Text key) { - return ProtobufHelper.getFixedByteString(key); - } - private static ByteString getFixedByteString(String key) { return ProtobufHelper.getFixedByteString(key); } From f8fc1b85b10e4023a1cf3fb519cb7864bf320eb7 Mon Sep 17 00:00:00 2001 From: Vinayakumar B Date: Mon, 13 Jan 2020 18:24:51 +0530 Subject: [PATCH 3/4] HADOOP-16621. [pb-upgrade] Remove Protobuf classes from signatures of Public APIs. Fixed Review comments --- .../java/org/apache/hadoop/ipc/ProtobufHelper.java | 14 +++++++------- .../org/apache/hadoop/security/Credentials.java | 4 ++-- .../hadoop/hdfs/protocolPB/PBHelperClient.java | 5 +++-- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java index bfd82cebefd06..faaf513190235 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java @@ -22,7 +22,7 @@ import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.io.Text; -import org.apache.hadoop.security.proto.SecurityProtos; +import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.security.token.TokenIdentifier; @@ -62,8 +62,8 @@ public static IOException getRemoteException(ServiceException se) { * This map should not be accessed directly. Used the getFixedByteString * methods instead. */ - private static ConcurrentHashMap fixedByteStringCache = - new ConcurrentHashMap<>(); + private final static ConcurrentHashMap + fixedByteStringCache = new ConcurrentHashMap<>(); /** * Get the ByteString for frequently used fixed and small set strings. @@ -98,8 +98,8 @@ public static ByteString getByteString(byte[] bytes) { return (bytes.length == 0) ? ByteString.EMPTY : ByteString.copyFrom(bytes); } - public static Token convert( - SecurityProtos.TokenProto tokenProto) { + public static Token tokenFromProto( + TokenProto tokenProto) { Token token = new Token<>( tokenProto.getIdentifier().toByteArray(), tokenProto.getPassword().toByteArray(), new Text(tokenProto.getKind()), @@ -107,8 +107,8 @@ public static Token convert( return token; } - public static SecurityProtos.TokenProto convert(Token tok) { - SecurityProtos.TokenProto.Builder builder = SecurityProtos.TokenProto.newBuilder(). + public static TokenProto protoFromToken(Token tok) { + TokenProto.Builder builder = TokenProto.newBuilder(). setIdentifier(getByteString(tok.getIdentifier())). setPassword(getByteString(tok.getPassword())). setKindBytes(getFixedByteString(tok.getKind())). diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java index 02a558ebb08aa..de30b18d648d7 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java @@ -369,7 +369,7 @@ void writeProto(DataOutput out) throws IOException { CredentialsKVProto.Builder kv = CredentialsKVProto.newBuilder(). setAliasBytes(ByteString.copyFrom( e.getKey().getBytes(), 0, e.getKey().getLength())). - setToken(ProtobufHelper.convert(e.getValue())); + setToken(ProtobufHelper.protoFromToken(e.getValue())); storage.addTokens(kv.build()); } @@ -391,7 +391,7 @@ void readProto(DataInput in) throws IOException { CredentialsProto storage = CredentialsProto.parseDelimitedFrom((DataInputStream)in); for (CredentialsKVProto kv : storage.getTokensList()) { addToken(new Text(kv.getAliasBytes().toByteArray()), - ProtobufHelper.convert(kv.getToken())); + ProtobufHelper.tokenFromProto(kv.getToken())); } for (CredentialsKVProto kv : storage.getSecretsList()) { addSecretKey(new Text(kv.getAliasBytes().toByteArray()), diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java index 7afa4994241e3..f8839ea8059a2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java @@ -324,7 +324,7 @@ public static ExtendedBlockProto convert(final ExtendedBlock b) { } public static TokenProto convert(Token tok) { - return ProtobufHelper.convert(tok); + return ProtobufHelper.protoFromToken(tok); } public static ShortCircuitShmIdProto convert(ShmId shmId) { @@ -802,7 +802,8 @@ public static StorageType[] convertStorageTypes( public static Token convert( TokenProto blockToken) { - return (Token) ProtobufHelper.convert(blockToken); + return (Token) ProtobufHelper + .tokenFromProto(blockToken); } // DatanodeId From 7e247581d9061c189b33e16a1a6f492d424711df Mon Sep 17 00:00:00 2001 From: Vinayakumar B Date: Tue, 14 Jan 2020 11:09:19 +0530 Subject: [PATCH 4/4] Fixed checkstyle comments --- .../java/org/apache/hadoop/ipc/ProtobufHelper.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java index faaf513190235..105628f19dcb2 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java @@ -63,7 +63,7 @@ public static IOException getRemoteException(ServiceException se) { * methods instead. */ private final static ConcurrentHashMap - fixedByteStringCache = new ConcurrentHashMap<>(); + FIXED_BYTESTRING_CACHE = new ConcurrentHashMap<>(); /** * Get the ByteString for frequently used fixed and small set strings. @@ -71,10 +71,10 @@ public static IOException getRemoteException(ServiceException se) { * @return */ public static ByteString getFixedByteString(Text key) { - ByteString value = fixedByteStringCache.get(key); + ByteString value = FIXED_BYTESTRING_CACHE.get(key); if (value == null) { value = ByteString.copyFromUtf8(key.toString()); - fixedByteStringCache.put(new Text(key.copyBytes()), value); + FIXED_BYTESTRING_CACHE.put(new Text(key.copyBytes()), value); } return value; } @@ -85,10 +85,10 @@ public static ByteString getFixedByteString(Text key) { * @return */ public static ByteString getFixedByteString(String key) { - ByteString value = fixedByteStringCache.get(key); + ByteString value = FIXED_BYTESTRING_CACHE.get(key); if (value == null) { value = ByteString.copyFromUtf8(key); - fixedByteStringCache.put(key, value); + FIXED_BYTESTRING_CACHE.put(key, value); } return value; }