HADOOP-16596. [pb-upgrade] Use shaded protobuf classes from hadoop-thirdparty dependency #1635
Conversation
This file contains 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
|
This has to be changed all modules together to avoid the compilation errors due to internal dependency on the hadoop-common's generated protobuf messages. So, expecting Yetus to complete precommit checks within 5 hrs(timeout) is not a feasible idea. |
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
b725973
to
570cd30
Compare
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
99630d2
to
7e24758
Compare
Summary of changesUsing hadoop-shaded-protobuf_3_7 artifact
replacer plugin to replace tokens (com.google.protobuf to o.a.h.thirdparty.protobuf_3_7)
Restored protobuf.version property to 2.5.0 and added to classpath.
Increased timeout in Jenkinsfile
|
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
7e24758
to
0aa1550
Compare
vinayakumarb
requested review from
steveloughran,
aajisaka,
jojochuang and
ayushtkn
|
💔 -1 overall
This message was automatically generated. |
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
2 times, most recently
from
a293e23
to
aa789f6
Compare
|
💔 -1 overall
This message was automatically generated. |
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
aa789f6
to
f9abf7a
Compare
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Big diff -but needs to go in everywhere.
- Its unfortunate it mixes org.apache import with others, but we had the same problem with the SLF4J update -we just have to accept it.
- should we have the protobuf version in the imports? A new upgrade will force a complete roll. What about "protobuf_current"?
maven
- switch to a property to drive testIgnore
The new exclusion from hadoop-common complicates life.
Modules should just be able to explicitly pull in hadoop-common and pick up the shaded protobuf JAR; no need to exclude the dependency there & then explicitly declare it later.
Yes, its a very good point, Thanks.
Anyway to change this, first need to change in hadoop-thirdparty module. |
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
f9abf7a
to
9c2ddbc
Compare
|
Updated with review comments fix. |
|
LGTM, +1 pending Steve's approval. I'm +1 for the option 2. Now I don't think both protobuf 3.x and protobuf 4.x shaded libs co-exist. We can upgrade the libs in all the modules at once. |
ayushtkn
approved these changes
Jan 28, 2020
There was a problem hiding this comment.
Thanx @vinayakumarb for the work here, Overall LGTM.
Optional Stuff : We can add in the POM test-sources also for the modules which doesn't use as of now like hdfs-client, would prevent extra effort of others understanding this change and doing similarlly in future if they tend to use in future.
Conflicts need to be resolved, Post Getting Build results, Do revert back the changes done just for build.
+1
…irdparty dependency
…irdparty dependency Replaced source files
…irdparty dependency Fixed checkstyle issues
…irdparty dependency 1. Ignoring test failures to continue to next module 2. Restoring protobuf.version property to 2.5.0 for backward compatibility, using hadoop.protobuf.version instead in pom xmls. 3. Fixed hbase timeline server tests by restoring 2.5.0 protobuf in classpath
…irdparty dependency increasing precommit run time
…irdparty dependency. Fixed review comments
…irdparty dependency. Resolved conflicts Added replace-test-sources
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16596
branch
from
3223754
to
e0701f6
Compare
|
Thanks @aajisaka and @ayushtkn for reviews. Thanks |
smengcl
reviewed
Jan 29, 2020
Jenkinsfile
Outdated
| @@ -23,7 +23,8 @@ pipeline { | |||
|
|
|||
| options { | |||
| buildDiscarder(logRotator(numToKeepStr: '5')) | |||
| timeout (time: 5, unit: 'HOURS') | |||
| //Increasing to 20 hours temporarily to allow precommit to run for all modules. | |||
| timeout (time: 20, unit: 'HOURS') | |||
There was a problem hiding this comment.
Shall we change this timeout to a lower value?
There was a problem hiding this comment.
yes. as said in earlier comments this is only temporary since this PR have changes in all modules and need lot of time.
will be restored back to 5 hours before final merge.
|
💔 -1 overall
This message was automatically generated. |
|
Hi @steveloughran, Please check the latest update and let us know are you fine with latest update? fixed all your comments. |
|
+1 from me |
steveloughran
approved these changes
Feb 6, 2020
aajisaka
approved these changes
Feb 7, 2020
|
Thanks @steveloughran and @aajisaka for reviews. |
|
Thank you for waiting. Tests passed on my local. +1 (I commented on https://issues.apache.org/jira/browse/HADOOP-16596 today.) |
|
Merged to trunk, Thanks Everyone for reviews. |
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
…irdparty dependency (apache#1635). Contributed by Vinayakumar B.
4 tasks
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.
Use the shaded protobuf classes from "hadoop-thirdparty" in hadoop codebase.
Depends on apache/hadoop-thirdparty#1 for the hadoop-thordparty dependency