[
https://issues.apache.org/jira/browse/HADOOP-18487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17759096#comment-17759096
]
ASF GitHub Bot commented on HADOOP-18487:
-----------------------------------------
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 @@
<!
> protobuf-2.5.0 dependencies => provided
> ---------------------------------------
>
> Key: HADOOP-18487
> URL: https://issues.apache.org/jira/browse/HADOOP-18487
> Project: Hadoop Common
> Issue Type: Improvement
> Components: build, ipc
> Affects Versions: 3.3.4
> Reporter: Steve Loughran
> Priority: Major
> Labels: pull-request-available
>
> uses of protobuf 2.5 and RpcEnginej have been deprecated since 3.3.0 in
> HADOOP-17046
> while still keeping those files around (for a long time...), how about we
> make the protobuf 2.5.0 export off hadoop common and hadoop-hdfs *provided*,
> rather than *compile*
> that way, if apps want it for their own apis, they have to explicitly ask for
> it, but at least our own scans don't break.
> i have no idea what will happen to the rest of the stack at this point, it
> will be "interesting" to see
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
