ayushtkn commented on code in PR #4996: URL: https://github.com/apache/hadoop/pull/4996#discussion_r1305758799
########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufWrapperLegacy.java: ########## @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ipc; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.util.Preconditions; + +/** + * A RpcWritable wrapper for unshaded protobuf messages. + * This class isolates unshaded protobuf classes from + * the rest of the RPC codebase, so it can operate without + * needing that on the classpath <i>at runtime</i>. + * The classes are needed at compile time; and if + * unshaded protobuf messages are to be marshalled, they + * will need to be on the classpath then. + * That is implicit: it is impossible to pass in a class + * which is a protobuf message unless that condition is met. + */ [email protected] Review Comment: misses ```InterfaceStability``` ########## hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml: ########## @@ -451,8 +451,7 @@ </Match> <Match> - <Class name="org.apache.hadoop.ipc.ProtobufHelper" /> - <Method name="getFixedByteString" /> + <Class name="org.apache.hadoop.ipc.internal.ShadedProtobufHelper" /> Review Comment: you want to exclude the entire class? not just one method ``getFixedByteString`` ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java: ########## @@ -30,31 +30,36 @@ import org.apache.hadoop.thirdparty.protobuf.ServiceException; /** - * Helper methods for protobuf related RPC implementation + * Helper methods for protobuf related RPC implementation. + * This is deprecated because it references protobuf 2.5 classes + * as well as the shaded ones -and so needs an unshaded protobuf-2.5 + * JAR on the classpath during execution. + * It MUST NOT be used internally; it is retained in case existing, + * external applications already use it. */ @InterfaceAudience.Private -public class ProtobufHelper { +@Deprecated Review Comment: It is deprecated, can we mention somewhere above like use ``ShadedProtobufHelper`` instead? ########## hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/pom.xml: ########## @@ -36,6 +36,10 @@ <groupId>org.apache.hadoop</groupId> <artifactId>hadoop-hdfs-client</artifactId> </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-common</artifactId> + </dependency> Review Comment: why? ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java: ########## @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ipc.internal; + +import java.io.IOException; +import java.util.concurrent.ConcurrentHashMap; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.io.Text; +import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; +import org.apache.hadoop.security.token.Token; +import org.apache.hadoop.security.token.TokenIdentifier; +import org.apache.hadoop.thirdparty.protobuf.ByteString; +import org.apache.hadoop.thirdparty.protobuf.ServiceException; + +/** + * Helper methods for protobuf related RPC implementation using the + * hadoop {@code org.apache.hadoop.thirdparty.protobuf} shaded version. + * This is <i>absolutely private to hadoop-* modules</i>. + */ [email protected] [email protected] +public final class ShadedProtobufHelper { + + private ShadedProtobufHelper() { + // Hidden constructor for class with only static helper methods + } + + /** + * Return the IOException thrown by the remote server wrapped in + * ServiceException as cause. + * The signature of this method changes with updates to the hadoop-thirdparty + * shaded protobuf library. + * @param se ServiceException that wraps IO exception thrown by the server + * @return Exception wrapped in ServiceException or + * a new IOException that wraps the unexpected ServiceException. + */ + @InterfaceAudience.Private + @InterfaceStability.Unstable + public static IOException getRemoteException(ServiceException se) { + Throwable e = se.getCause(); + if (e == null) { + return new IOException(se); + } + return e instanceof IOException + ? (IOException) e + : new IOException(se); Review Comment: nit it can be in one single line, this split line doesn't look very readable to me ``` return e instanceof IOException ? (IOException) e : new IOException(se); ``` ########## hadoop-project/pom.xml: ########## @@ -85,8 +85,14 @@ <!-- com.google.re2j version --> <re2j.version>1.1</re2j.version> - <!--Protobuf version for backward compatibility--> + <!-- Protobuf version for backward compatibility --> + <!-- This is used in hadoop-common for compilation only --> <protobuf.version>2.5.0</protobuf.version> + <!-- Protobuf scope in hadoop common --> + <!-- set to "provided" and protobuf2 will no longer be exported as a dependency --> + <common.protobuf2.scope>compile</common.protobuf2.scope> + <!-- Protobuf scope in other modules which explicitly import the libarary --> + <transient.protobuf2.scope>${common.protobuf2.scope}</transient.protobuf2.scope> Review Comment: you hardcoded compile, how do you test later that the compilation doesn't break by any change in future? may be a github action just doing the build with ``provided``? ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java: ########## @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ipc.internal; + +import java.io.IOException; +import java.util.concurrent.ConcurrentHashMap; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.io.Text; +import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; +import org.apache.hadoop.security.token.Token; +import org.apache.hadoop.security.token.TokenIdentifier; +import org.apache.hadoop.thirdparty.protobuf.ByteString; +import org.apache.hadoop.thirdparty.protobuf.ServiceException; + +/** + * Helper methods for protobuf related RPC implementation using the + * hadoop {@code org.apache.hadoop.thirdparty.protobuf} shaded version. + * This is <i>absolutely private to hadoop-* modules</i>. + */ [email protected] [email protected] +public final class ShadedProtobufHelper { + + private ShadedProtobufHelper() { + // Hidden constructor for class with only static helper methods + } + + /** + * Return the IOException thrown by the remote server wrapped in + * ServiceException as cause. + * The signature of this method changes with updates to the hadoop-thirdparty + * shaded protobuf library. + * @param se ServiceException that wraps IO exception thrown by the server + * @return Exception wrapped in ServiceException or + * a new IOException that wraps the unexpected ServiceException. + */ + @InterfaceAudience.Private + @InterfaceStability.Unstable Review Comment: Why this is required here? it will inherit from the class? ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java: ########## @@ -30,31 +30,36 @@ import org.apache.hadoop.thirdparty.protobuf.ServiceException; /** - * Helper methods for protobuf related RPC implementation + * Helper methods for protobuf related RPC implementation. + * This is deprecated because it references protobuf 2.5 classes + * as well as the shaded ones -and so needs an unshaded protobuf-2.5 + * JAR on the classpath during execution. + * It MUST NOT be used internally; it is retained in case existing, + * external applications already use it. */ @InterfaceAudience.Private -public class ProtobufHelper { +@Deprecated +public final class ProtobufHelper { Review Comment: I thought you kept this class to maintain compat for anyone who is using this class, but earlier this class wasn't ``final`` is that intentional? ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java: ########## @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ipc.internal; + +import java.io.IOException; +import java.util.concurrent.ConcurrentHashMap; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.io.Text; +import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; +import org.apache.hadoop.security.token.Token; +import org.apache.hadoop.security.token.TokenIdentifier; +import org.apache.hadoop.thirdparty.protobuf.ByteString; +import org.apache.hadoop.thirdparty.protobuf.ServiceException; + +/** + * Helper methods for protobuf related RPC implementation using the + * hadoop {@code org.apache.hadoop.thirdparty.protobuf} shaded version. + * This is <i>absolutely private to hadoop-* modules</i>. + */ [email protected] [email protected] +public final class ShadedProtobufHelper { + + private ShadedProtobufHelper() { + // Hidden constructor for class with only static helper methods + } + + /** + * Return the IOException thrown by the remote server wrapped in + * ServiceException as cause. + * The signature of this method changes with updates to the hadoop-thirdparty + * shaded protobuf library. + * @param se ServiceException that wraps IO exception thrown by the server + * @return Exception wrapped in ServiceException or + * a new IOException that wraps the unexpected ServiceException. + */ + @InterfaceAudience.Private + @InterfaceStability.Unstable + public static IOException getRemoteException(ServiceException se) { + Throwable e = se.getCause(); + if (e == null) { + return new IOException(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. + * <p> + * This map should not be accessed directly. Used the getFixedByteString + * methods instead. + */ + private static final ConcurrentHashMap<Object, ByteString> + FIXED_BYTESTRING_CACHE = new ConcurrentHashMap<>(); + + /** + * Get the ByteString for frequently used fixed and small set strings. + * @param key string + * @return the ByteString for frequently used fixed and small set strings. + */ + public static ByteString getFixedByteString(Text key) { + ByteString value = FIXED_BYTESTRING_CACHE.get(key); + if (value == null) { + value = ByteString.copyFromUtf8(key.toString()); + FIXED_BYTESTRING_CACHE.put(new Text(key.copyBytes()), value); + } + return value; + } + + /** + * Get the ByteString for frequently used fixed and small set strings. + * @param key string + * @return ByteString for frequently used fixed and small set strings. + */ + public static ByteString getFixedByteString(String key) { + ByteString value = FIXED_BYTESTRING_CACHE.get(key); + if (value == null) { + value = ByteString.copyFromUtf8(key); + FIXED_BYTESTRING_CACHE.put(key, value); + } + return value; + } + + /** + * Get the byte string of a non-null byte array. + * If the array is 0 bytes long, return a singleton to reduce object allocation. + * @param bytes bytes to convert. + * @return a value Review Comment: a value ain't very clear :-(, but ok ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java: ########## @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ipc.internal; + +import java.io.IOException; +import java.util.concurrent.ConcurrentHashMap; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.io.Text; +import org.apache.hadoop.security.proto.SecurityProtos.TokenProto; +import org.apache.hadoop.security.token.Token; +import org.apache.hadoop.security.token.TokenIdentifier; +import org.apache.hadoop.thirdparty.protobuf.ByteString; +import org.apache.hadoop.thirdparty.protobuf.ServiceException; + +/** + * Helper methods for protobuf related RPC implementation using the + * hadoop {@code org.apache.hadoop.thirdparty.protobuf} shaded version. + * This is <i>absolutely private to hadoop-* modules</i>. + */ [email protected] [email protected] +public final class ShadedProtobufHelper { + + private ShadedProtobufHelper() { + // Hidden constructor for class with only static helper methods + } + + /** + * Return the IOException thrown by the remote server wrapped in + * ServiceException as cause. + * The signature of this method changes with updates to the hadoop-thirdparty + * shaded protobuf library. + * @param se ServiceException that wraps IO exception thrown by the server + * @return Exception wrapped in ServiceException or + * a new IOException that wraps the unexpected ServiceException. + */ + @InterfaceAudience.Private + @InterfaceStability.Unstable + public static IOException getRemoteException(ServiceException se) { + Throwable e = se.getCause(); + if (e == null) { + return new IOException(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. + * <p> + * This map should not be accessed directly. Used the getFixedByteString + * methods instead. + */ + private static final ConcurrentHashMap<Object, ByteString> + FIXED_BYTESTRING_CACHE = new ConcurrentHashMap<>(); + + /** + * Get the ByteString for frequently used fixed and small set strings. + * @param key string Review Comment: this key is ``Text``, in the other method it is string -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
