-
Notifications
You must be signed in to change notification settings - Fork 52
HBASE-29303 [hbase-thirdparty] Bump protobuf java to 4.30.2 #136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Compilation will fail as variable I will try to modify the existing patch https://github.com/apache/hbase-thirdparty/blob/master/hbase-shaded-protobuf/src/main/patches/HBASE-15789_V3.patch which requires this field! |
Second commit handles this issue! |
|
💔 -1 overall
This message was automatically generated. |
Failed, as expected! |
This comment was marked as outdated.
This comment was marked as outdated.
|
Is the protobuf fork we're generating the patch from available somewhere ( i.e. github.com )? |
Yes please refer README.md of thirdparty repo. I added steps to generate patch from protobuf clone during last release. For this PR, I checked out v30.2. |
|
💔 -1 overall
This message was automatically generated. |
|
Strange even though I have renamed the patch file it is still applying old one not sure how. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
|
Finally a Pass 😌 Hi @stoty , @Apache9 does this look good? Should I go ahead run this PR and #135 against hbase master branch? |
|
|
||
| ``` | ||
| HBASE-15789_V3.patch | ||
| HBASE-15789_V4.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure versioning this patch is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stoty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
|
Sure, running a full CI build is the only way to validate. |
Build passes with this change, refer apache/hbase#6993 |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch management is so tedious.
…d4bfe in HBASE-15789_V3.patch * Also rename HBASE-15789_V3.patch to HBASE-15789_V4.patch
|
🎊 +1 overall
This message was automatically generated. |
Yes, it really is. Let me raise a JIRA to at least change the requirement to rename a patch as @stoty also suggested seems to be not much useful. |
Uh oh!
There was an error while loading. Please reload this page.