HADOOP-18487. Make protobuf 2.5 an optional runtime dependency. (#4996)

Protobuf 2.5 JAR is no longer needed at runtime.

The option common.protobuf.scope defines whether the protobuf 2.5.0
dependency is marked as provided or not.

* New package org.apache.hadoop.ipc.internal for internal only protobuf classes
  ...with a ShadedProtobufHelper in there which has shaded protobuf refs
  only, so guaranteed not to need protobuf-2.5 on the CP
* All uses of org.apache.hadoop.ipc.ProtobufHelper have
  been replaced by uses of org.apache.hadoop.ipc.internal.ShadedProtobufHelper
* The scope of protobuf-2.5 is set by the option common.protobuf2.scope
  In this patch is it is still "compile"
* There is explicit reference to it in modules where it may be needed.
*  The maven scope of the dependency can be set with the common.protobuf2.scope
   option. It can be set to "provided" in a build:
       -Dcommon.protobuf2.scope=provided
* Add new ipc(callable) method to catch and convert shaded protobuf
  exceptions raised during invocation of the supplied lambda expression
* This is adopted in the code where the migration is not traumatically
  over-complex. RouterAdminProtocolTranslatorPB is left alone for this
  reason.

Contributed by Steve Loughran

Author: steveloughran

BUILDING.txt

@@ -293,6 +293,30 @@ Maven build goals:
     package. This option requires that -Dpmdk.lib is specified. With -Dbundle.pmdk provided,
     the build will fail if -Dpmdk.lib is not specified.
 
+Controlling the redistribution of the protobuf-2.5 dependency
+
+    The protobuf 2.5.0 library is used at compile time to compile the class
+    org.apache.hadoop.ipc.ProtobufHelper; this class known to have been used by
+    external projects in the past. Protobuf 2.5 is not used elsewhere in
+    the Hadoop codebase; alongside the move to Protobuf 3.x a private successor
+    class, org.apache.hadoop.ipc.internal.ShadedProtobufHelper is now used.
+
+    The hadoop-common JAR still declares a dependency on protobuf-2.5, but this
+    is likely to change in the future. The maven scope of the dependency can be
+    set with the common.protobuf2.scope option.
+    It can be set to "provided" in a build:
+       -Dcommon.protobuf2.scope=provided
+    If this is done then protobuf-2.5.0.jar will no longer be exported as a dependency,
+    and will then be omitted from the share/hadoop/common/lib/ directory of
+    any Hadoop distribution built. Any application declaring a dependency on hadoop-commmon
+    will no longer get the dependency; if they need it then they must explicitly declare it:
+
+      <dependency>
+        <groupId>com.google.protobuf</groupId>
+        <artifactId>protobuf-java</artifactId>
+        <version>2.5.0</version>
+      </dependency>
+
 ----------------------------------------------------------------------------------
 Building components separately
 

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" />
     <Bug pattern="AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION" />
   </Match>
 </FindBugsFilter>

hadoop-common-project/hadoop-common/pom.xml

@@ -250,10 +250,11 @@
       <artifactId>re2j</artifactId>
       <scope>compile</scope>
     </dependency>
+    <!-- Needed for compilation, though no longer in production.  -->
     <dependency>
       <groupId>com.google.protobuf</groupId>
       <artifactId>protobuf-java</artifactId>
-      <scope>compile</scope>
+      <scope>${common.protobuf2.scope}</scope>
     </dependency>
     <dependency>
       <groupId>com.google.code.gson</groupId>
@@ -484,11 +485,11 @@
               <!--These classes have direct Protobuf references for backward compatibility reasons-->
               <excludes>
                 <exclude>**/ProtobufHelper.java</exclude>
-                <exclude>**/RpcWritable.java</exclude>
                 <exclude>**/ProtobufRpcEngineCallback.java</exclude>
                 <exclude>**/ProtobufRpcEngine.java</exclude>
                 <exclude>**/ProtobufRpcEngine2.java</exclude>
                 <exclude>**/ProtobufRpcEngineProtos.java</exclude>
+                <exclude>**/ProtobufWrapperLegacy.java</exclude>
               </excludes>
             </configuration>
           </execution>

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/protocolPB/HAServiceProtocolClientSideTranslatorPB.java

@@ -37,14 +37,13 @@
 import org.apache.hadoop.ha.proto.HAServiceProtocolProtos.TransitionToActiveRequestProto;
 import org.apache.hadoop.ha.proto.HAServiceProtocolProtos.TransitionToStandbyRequestProto;
 import org.apache.hadoop.ha.proto.HAServiceProtocolProtos.TransitionToObserverRequestProto;
-import org.apache.hadoop.ipc.ProtobufHelper;
 import org.apache.hadoop.ipc.ProtobufRpcEngine2;
 import org.apache.hadoop.ipc.ProtocolTranslator;
 import org.apache.hadoop.ipc.RPC;
 import org.apache.hadoop.security.UserGroupInformation;
-
 import org.apache.hadoop.thirdparty.protobuf.RpcController;
-import org.apache.hadoop.thirdparty.protobuf.ServiceException;
+
+import static org.apache.hadoop.ipc.internal.ShadedProtobufHelper.ipc;
 
 /**
  * This class is the client side translator to translate the requests made on
@@ -84,60 +83,39 @@ public HAServiceProtocolClientSideTranslatorPB(
 
   @Override
   public void monitorHealth() throws IOException {
-    try {
-      rpcProxy.monitorHealth(NULL_CONTROLLER, MONITOR_HEALTH_REQ);
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    ipc(() -> rpcProxy.monitorHealth(NULL_CONTROLLER, MONITOR_HEALTH_REQ));
   }
 
   @Override
   public void transitionToActive(StateChangeRequestInfo reqInfo) throws IOException {
-    try {
-      TransitionToActiveRequestProto req =
-          TransitionToActiveRequestProto.newBuilder()
+    TransitionToActiveRequestProto req =
+        TransitionToActiveRequestProto.newBuilder()
             .setReqInfo(convert(reqInfo)).build();
-
-      rpcProxy.transitionToActive(NULL_CONTROLLER, req);
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    ipc(() -> rpcProxy.transitionToActive(NULL_CONTROLLER, req));
   }
 
   @Override
   public void transitionToStandby(StateChangeRequestInfo reqInfo) throws IOException {
-    try {
-      TransitionToStandbyRequestProto req =
+    TransitionToStandbyRequestProto req =
         TransitionToStandbyRequestProto.newBuilder()
-          .setReqInfo(convert(reqInfo)).build();
-      rpcProxy.transitionToStandby(NULL_CONTROLLER, req);
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+            .setReqInfo(convert(reqInfo)).build();
+    ipc(() -> rpcProxy.transitionToStandby(NULL_CONTROLLER, req));
   }
 
   @Override
   public void transitionToObserver(StateChangeRequestInfo reqInfo)
       throws IOException {
-    try {
-      TransitionToObserverRequestProto req =
-          TransitionToObserverRequestProto.newBuilder()
-              .setReqInfo(convert(reqInfo)).build();
-      rpcProxy.transitionToObserver(NULL_CONTROLLER, req);
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    TransitionToObserverRequestProto req =
+        TransitionToObserverRequestProto.newBuilder()
+            .setReqInfo(convert(reqInfo)).build();
+    ipc(() -> rpcProxy.transitionToObserver(NULL_CONTROLLER, req));
   }
 
   @Override
   public HAServiceStatus getServiceStatus() throws IOException {
     GetServiceStatusResponseProto status;
-    try {
-      status = rpcProxy.getServiceStatus(NULL_CONTROLLER,
-          GET_SERVICE_STATUS_REQ);
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    status = ipc(() -> rpcProxy.getServiceStatus(NULL_CONTROLLER,
+        GET_SERVICE_STATUS_REQ));
 
     HAServiceStatus ret = new HAServiceStatus(
         convert(status.getState()));

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ha/protocolPB/ZKFCProtocolClientSideTranslatorPB.java

@@ -27,15 +27,14 @@
 import org.apache.hadoop.ha.ZKFCProtocol;
 import org.apache.hadoop.ha.proto.ZKFCProtocolProtos.CedeActiveRequestProto;
 import org.apache.hadoop.ha.proto.ZKFCProtocolProtos.GracefulFailoverRequestProto;
-import org.apache.hadoop.ipc.ProtobufHelper;
 import org.apache.hadoop.ipc.ProtobufRpcEngine2;
 import org.apache.hadoop.ipc.ProtocolTranslator;
 import org.apache.hadoop.ipc.RPC;
 import org.apache.hadoop.security.AccessControlException;
 import org.apache.hadoop.security.UserGroupInformation;
-
 import org.apache.hadoop.thirdparty.protobuf.RpcController;
-import org.apache.hadoop.thirdparty.protobuf.ServiceException;
+
+import static org.apache.hadoop.ipc.internal.ShadedProtobufHelper.ipc;
 
 
 public class ZKFCProtocolClientSideTranslatorPB implements
@@ -57,24 +56,16 @@ public ZKFCProtocolClientSideTranslatorPB(
   @Override
   public void cedeActive(int millisToCede) throws IOException,
       AccessControlException {
-    try {
-      CedeActiveRequestProto req = CedeActiveRequestProto.newBuilder()
-          .setMillisToCede(millisToCede)
-          .build();
-      rpcProxy.cedeActive(NULL_CONTROLLER, req);      
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    CedeActiveRequestProto req = CedeActiveRequestProto.newBuilder()
+        .setMillisToCede(millisToCede)
+        .build();
+    ipc(() -> rpcProxy.cedeActive(NULL_CONTROLLER, req));
   }
 
   @Override
   public void gracefulFailover() throws IOException, AccessControlException {
-    try {
-      rpcProxy.gracefulFailover(NULL_CONTROLLER,
-          GracefulFailoverRequestProto.getDefaultInstance());
-    } catch (ServiceException e) {
-      throw ProtobufHelper.getRemoteException(e);
-    }
+    ipc(() -> rpcProxy.gracefulFailover(NULL_CONTROLLER,
+        GracefulFailoverRequestProto.getDefaultInstance()));
   }
 
 

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java

@@ -18,10 +18,10 @@
 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.ipc.internal.ShadedProtobufHelper;
 import org.apache.hadoop.security.proto.SecurityProtos.TokenProto;
 import org.apache.hadoop.security.token.Token;
 import org.apache.hadoop.security.token.TokenIdentifier;
@@ -30,31 +30,37 @@
 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.
+ * @deprecated hadoop code MUST use {@link ShadedProtobufHelper}.
  */
 @InterfaceAudience.Private
+@Deprecated
 public class ProtobufHelper {
+
   private ProtobufHelper() {
     // Hidden constructor for class with only static helper methods
   }
 
   /**
-   * Return the IOException thrown by the remote server wrapped in 
+   * Return the IOException thrown by the remote server wrapped in
    * ServiceException as cause.
    * @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.
    */
   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);
+    return ShadedProtobufHelper.getRemoteException(se);
   }
 
   /**
-   * Kept for backward compatible.
+   * Extract the remote exception from an unshaded version of the protobuf
+   * libraries.
+   * Kept for backward compatibility.
    * Return the IOException thrown by the remote server wrapped in
    * ServiceException as cause.
    * @param se ServiceException that wraps IO exception thrown by the server
@@ -71,29 +77,13 @@ public static IOException getRemoteException(
     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 final static 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;
+    return ShadedProtobufHelper.getFixedByteString(key);
   }
 
   /**
@@ -102,34 +92,40 @@ public static ByteString getFixedByteString(Text key) {
    * @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;
+    return ShadedProtobufHelper.getFixedByteString(key);
   }
 
+  /**
+   * 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
+   */
   public static ByteString getByteString(byte[] bytes) {
     // return singleton to reduce object allocation
-    return (bytes.length == 0) ? ByteString.EMPTY : ByteString.copyFrom(bytes);
+    return ShadedProtobufHelper.getByteString(bytes);
   }
 
+  /**
+   * Get a token from a TokenProto payload.
+   * @param tokenProto marshalled token
+   * @return the token.
+   */
   public static Token<? extends TokenIdentifier> tokenFromProto(
       TokenProto tokenProto) {
-    Token<? extends TokenIdentifier> token = new Token<>(
-        tokenProto.getIdentifier().toByteArray(),
-        tokenProto.getPassword().toByteArray(), new Text(tokenProto.getKind()),
-        new Text(tokenProto.getService()));
-    return token;
+    return ShadedProtobufHelper.tokenFromProto(tokenProto);
   }
 
+  /**
+   * Create a {@code TokenProto} instance
+   * from a hadoop token.
+   * This builds and caches the fields
+   * (identifier, password, kind, service) but not
+   * renewer or any payload.
+   * @param tok token
+   * @return a marshallable protobuf class.
+   */
   public static TokenProto protoFromToken(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 ShadedProtobufHelper.protoFromToken(tok);
   }
 }