HADOOP-18487. protobuf 2.5.0 marked as provided. #4996
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
having just seen #2026 i'm not convinced we can remove protobuf2.5 off the classpath of anything using the RPC classes. There's a lot of internal probing for object types and dynamic choice of shaded/unshaded classes. It might be possible to pull out the code from the shared classes (server, rpcwritable...) so be confident that rpc clients only using the shaded libraries and the RpcEngine2 were safely isolated, but right now I am not convinced that is true. Certainly hdfs, mr and yarn servers and clients MUST export protobuf2, even if hadoop-common downgrades it. |
|
we can't cut an unshaded protobuf of some form without RPC not linking, so hbase/hive/ozone are in trouble here. changes to RPC.java required to somehow add ability to probe a class for being a subclass of com.google.protobuf.Message without having com.google.protobuf.Message on the classpath would be needed. this does not need to be protobuf 2.5. I propose
|
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
c837155 to
8309ad3
Compare
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
2 times, most recently
from
e6c4fb9 to
dad1ddb
Compare
|
...not targeting 3.3.5 for this; not a blocker |
4 tasks
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
79fd97a to
ad02499
Compare
|
spotbugs still unhappy. will have to explicitly exclude |
4 tasks
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
def2282 to
ca82091
Compare
|
💔 -1 overall
This message was automatically generated. |
|
test Failures are all in TestMRJobsWithProfiler, covered exactly in MAPREDUCE-7436 and therefore unrelated. |
|
rbf compile failure is very much related |
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
ca82091 to
6ce5309
Compare
|
tests are unrelated. this pr is ready to go in, followed by one switching protobuf 2.5 to provided |
pjfanning
approved these changes
Aug 21, 2023
| * Helper methods for protobuf related RPC implementation. | ||
| * This is deprecated because it references protobuf 2.5 classes | ||
| * as well as the shaded one -and may need an unshaded protobuf | ||
| * JAR on the classpath during complicated. |
There was a problem hiding this comment.
during compilation - instead complicated
|
@pjfanning thanks -reviewed all the javadocs and corrected any issues I could see, made sure everything had a doc and used MUST NOT USE in the class javadoc. |
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
6ce5309 to
74b2a60
Compare
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Aug 25, 2023
There was a problem hiding this comment.
Minor stuff nothing major, you can ignore whichever you want. I tried compiling locally with provided scope & it is all calm.
Do you expect any complications in Old client new Server scenarios? If so, I can quickly do some basic tests by deploying your branch and try a 3.3.6 client.
In general looks good at least to me, but I will try some normal tests post deploying this locally to see how things are
...-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufWrapperLegacy.java
Show resolved
Hide resolved
...project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java
Outdated
Show resolved
Hide resolved
Comment on lines
+93
to
+94
| <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> |
There was a problem hiding this 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?
There was a problem hiding this comment.
i have a future plan to move to provided, but by making it a separate patch it becomes easier to revert if needed
...project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/ProtobufHelper.java
Outdated
Show resolved
Hide resolved
...project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java
Outdated
Show resolved
Hide resolved
...project/hadoop-common/src/main/java/org/apache/hadoop/ipc/internal/ShadedProtobufHelper.java
Outdated
Show resolved
Hide resolved
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/pom.xml
Show resolved
Hide resolved
|
Ohh, I forgot some: |
now, if we make it a switch, should i do it in this pr or that followup I was thinking about |
|
anything is good with me, can do in the followup pr as well |
HBase branch-3 and trunk are fine, we use our own shaded protobuf everywhere. The major version increment allowed it. For HBase branch-2, all the 2.x releases, and Apache Phoenix, the com.google.protobuf API classes are part of the RPC side of the Coprocessor API so indeed these would all be in trouble in theory. Internally even in 2.x releases we use protobuf 3 for actual RPC. The PB 2.5 classes are only required due to interface compatibility constraints and therefore its on us to ensure the dependencies are imported. We should not (and do not) depend on Hadoop providing an explicit export of PB 2.5 anyway. When I made a similar change where I work in an internal fork of Hadoop 3 I removed the legacy RPC classes ( |
apurtell
approved these changes
Aug 28, 2023
|
|
||
| import org.apache.hadoop.thirdparty.protobuf.RpcController; | ||
| import org.apache.hadoop.thirdparty.protobuf.ServiceException; | ||
|
|
||
| import static org.apache.hadoop.ipc.internal.ShadedProtobufHelper.getRemoteException; |
There was a problem hiding this comment.
A minor nit. There is an inconsistency where the other helper methods in ShadedProtobufHelper are invoked without static import, but for some reason getRemoteException() is an exception. :-)
There was a problem hiding this comment.
switched back in this class. generally i was trying to make the diff smaller; must have got a bit over-enthusiastic here.
There was a problem hiding this comment.
actually, done something more elegant
Comment on lines
+93
to
+94
| <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> |
|
updated pr tries to address reviews, including building.txt details. regarding @apurtell's comment about static vs qualified import of public static <T> T ipc(IpcCall<T> call) throws IOException {
try {
return call.call();
} catch (ServiceException e) {
throw getRemoteException(e);
}
}this lets us have far simpler invocations in the code status = ipc(() -> rpcProxy.getServiceStatus(NULL_CONTROLLER,
GET_SERVICE_STATUS_REQ));I've done this for hadoop-common; if all are happy then I will do for the rest of the modules |
ayushtkn
approved these changes
Sep 12, 2023
There was a problem hiding this comment.
Thanx @steveloughran for the update. If the build comes green. Changes LGTM.
Regarding getRemoteException() I don't have any strong opinions around any approach, just be consistent through out the project.
Feel free to commit once you have a green build and you sort out getRemoteException for the entire project
|
💔 -1 overall
This message was automatically generated. |
|
@ayushtkn thanks. i will do it everywhere; its the same as the s3a invoker stuff and it is more elegant, especially for anyone writing new rpc stuff |
|
💔 -1 overall
This message was automatically generated. |
…ency.
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.
steveloughran
force-pushed
the
feature/HADOOP-18487-downgrade-protobuf-2.5
branch
from
12a0cca to
a70ceff
Compare
|
rebased for a retest, then will merge. |
|
💔 -1 overall
This message was automatically generated. |
|
test failure seems unrelated. javadocs maybe its the : |
Change-Id: I775e4836a61be60c2bb4c8c1c0133bef43111b5b
|
💔 -1 overall
This message was automatically generated. |
|
dfs upgrade test is flaky (and has a history), created HDFS-17224 to cover the new failure and verified it works standalone |
steveloughran
added a commit
to steveloughran/hadoop
that referenced
this pull request
Oct 13, 2023
…he#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 Change-Id: I7268580c3a6b40a1a72de79e36de58d2efb0b76e
4 tasks
steveloughran
added a commit
that referenced
this pull request
Oct 17, 2023
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
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…he#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
prabhjyotsingh
pushed a commit
to acceldata-io/hadoop
that referenced
this pull request
Mar 11, 2025
…ncy. (apache#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 (cherry picked from commit 48b37a7)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The option protobuf.scope defines whether the protobuf 2.5.0 dependency is marked as provided or not.
It's actually interesting to see where/how that compile fails
hadoop-hdfs-client: ClientNamenodeProtocolTranslatorPB hadoop-hdfs-rbf:RouterAdminProtocolTranslatorPB
both with "class file for com.google.protobuf.ServiceException not found", even though neither class uses it
what they do have is references to ProtobufHelper.getRemoteException(), which is overloaded to both the shaded ServiceException and the original one
Hypothesis: the javac overload resolution needs to look at the entire class hierarchy before it can decide which one to use.
Proposed: add a new method
ioe extractException(org.apache.hadoop.thirdparty.protobuf.ServiceException)
and move our own code to it. Without the overloading the classes should not be needed
Change-Id: I70354abfe3f1fdc03c418dac88e60f8cc4929a33
How was this patch tested?
local compile, now looking at yetus runs
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?