HBASE-28725 Use thirdparty protobuf for REST interface in HBase 2.x #6075
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
Contributor
Author
|
Note that is includes duplicating the protobuf generated file shading setup from hbase-shaded-protocol. Another option is to move the protobuf processing to hbase-shaded-protocol, and avoid doing the shading in two modules. (as done on branch-3+) |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 12, 2024
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
In general I think the approach is good.
The only concern is about copying maven plugin with a lot of special configs...
| </systemPropertyVariables> | ||
| </configuration> | ||
| </plugin> | ||
| <!-- The protobuf-maven-plugin and com.google.code.maven-replacer-plugin config |
Contributor
There was a problem hiding this comment.
Could we move this to parent pom too? So we do not need to copy paste it everywhere...
Contributor
Author
There was a problem hiding this comment.
I have centrilzed the replace plugin.
The protobuf compiler config cannot be centralized, as 2.x uses both protobuf 2.5 and thirdparty compilers.
Apache9
reviewed
Jul 12, 2024
| </goals> | ||
| <phase>generate-sources</phase> | ||
| <configuration> | ||
| <protocArtifact>com.google.protobuf:protoc:${internal.protobuf.version}:exe:${os.detected.classifier}</protocArtifact> |
Contributor
There was a problem hiding this comment.
I think the only difference is this configuration? We could introduce a property for this so in hbase-protocol-shade and hbase-reset, we use the new protoc compiler while in hbase-protocol, we use the old one?
Contributor
Author
There was a problem hiding this comment.
Yes, we could, but I think that the would hide the difference, and make it even harder to find which version we use where.
All the different protobuf libraries and compilers are hard enough figure out already, the current setup at least gives a clue to whoever tries to decypher it.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Please fix the javadoc issues? |
Contributor
Author
|
The reported javadoc issues are in the generated protobuf code, @Apache9 . We are also excluding generated file in the report generation javadoc phase, but that's not an option hbase-rest. Should I add the same exclude settings to the normal (build) javadoc settings in the root pom ? |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
I have moved the javadoc exclusions from the report section to the dependencyManagement section, so that they also apply in the build section. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
rebased to current branch-2 |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Can you take another look, @Apache9 ? |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.