HBASE-23833. The relocated hadoop-thirdparty protobuf breaks HBase asyncwal #1301
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
(cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
reviewed
Mar 19, 2020
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
Arcane but looks like it would work. Some comments in the below. Thanks @jojochuang
...src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutputSaslHelper.java
Show resolved
Hide resolved
| // Was ByteStringer; fix w/o using ByteStringer. Its in hbase-protocol | ||
| // and we want to keep that out of hbase-server. | ||
| builder.setPayload(ByteString.copyFrom(payload)); | ||
| BuilderPayloadSetter.setter(builder, payload); |
Contributor
There was a problem hiding this comment.
The builder is from HDFS. It is NOT a pb. It has pbs in it?
|
|
||
| static { | ||
| builderClass = DataTransferEncryptorMessageProto.Builder.class; | ||
| byteStringClass = com.google.protobuf.ByteString.class; |
Contributor
There was a problem hiding this comment.
Is this ok? This presumes what pb is on the CLASSPATH? pb2.5? Should it be the shaded hbase ByteString?
Contributor
There was a problem hiding this comment.
Or this is just a default setting?
Contributor
There was a problem hiding this comment.
Seems like it should be hbase internal shaded ByteString, not this com.google one.
Contributor
There was a problem hiding this comment.
oh. i get it. This is the HDFS ByteString. The one that will be on the CP for all versions before HDFS3.3. Ok.
...src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutputSaslHelper.java
Outdated
Show resolved
Hide resolved
| static void setter(DataTransferEncryptorMessageProto.Builder builder, byte[] payload) { | ||
| Object byteStringObject = null; | ||
| try { | ||
| byteStringObject = copyFromMethod.invoke(null, payload); |
Contributor
There was a problem hiding this comment.
This a byte array copy? If we wanted to wrap w/o copy, could we do that?
Contributor
Author
There was a problem hiding this comment.
yeah i think that's possible. In fact, that was the case prior to HBASE-17908. Here the code simply do what was there after HBASE-17908. I can certain try do the zero copy thing too.
...src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutputSaslHelper.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/asyncfs/ProtobufDecoder.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/asyncfs/ProtobufDecoder.java
Outdated
Show resolved
Hide resolved
1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time.
Contributor
Author
|
Thanks @saintstack. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The spotbugs error is unrelated, existing bug. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
Apr 8, 2020
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
Lets try it (Sorry, forgot about it).
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…yncwal (apache#1301) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…yncwal (apache#1301) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…yncwal (apache#1301) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]>
jojochuang
added a commit
to jojochuang/hbase
that referenced
this pull request
Apr 16, 2020
…yncwal (apache#1301) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff)
jojochuang
added a commit
to jojochuang/hbase
that referenced
this pull request
Apr 16, 2020
…yncwal (apache#1301) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38)
saintstack
pushed a commit
that referenced
this pull request
Apr 17, 2020
…yncwal (#1301) (#1534) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff)
saintstack
pushed a commit
that referenced
this pull request
Apr 17, 2020
…yncwal (#1301) (#1535) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38)
ddupg
pushed a commit
to ddupg/hbase
that referenced
this pull request
Apr 20, 2020
…yncwal (apache#1301) (apache#1535) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38)
jojochuang
added a commit
to jojochuang/hbase
that referenced
this pull request
Apr 23, 2020
…yncwal (apache#1301) (apache#1535) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38) (cherry picked from commit b8b8e0a)
saintstack
pushed a commit
that referenced
this pull request
Apr 29, 2020
…yncwal (#1301) (#1535) (#1567) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38) (cherry picked from commit b8b8e0a)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…yncwal (apache#1301) (apache#1535) (apache#1567) * Use Reflection to access shaded Hadoop protobuf classes. (cherry picked from commit a321e536989083ca3620bf2c53f12c07740bf5b0) * Update to improve the code: 1. Added license. 2. Added more comments. 3. Wrap byte array instead of copy to make a ByteString. 4. Moved all reflection instantiation to static class loading time. * Use LiteralByteString to wrap byte array instead of copying it. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: stack <[email protected]> (cherry picked from commit 72727ff) (cherry picked from commit ae3de38) (cherry picked from commit b8b8e0a) (cherry picked from commit 6a2314d) Change-Id: Id87f30e022d36acbd629364e01e952954cce4460
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
I've tried a few approaches. It turns out the quickest solution to this is with Java Reflection.
(1) Create a ProtobufDecoder that is inspired by io.netty.handler.codec.protobuf.ProtobufDecoder. The original ProtobufDecoder has dependency on protobuf. Use reflection to access the shaded protobuf in HDFS when applicable.
(2) Similarly, create a private class BuilderPayloadSetter that does ByteString.copyFrom() + DataTransferEncryptorMessageProto.Builder.setPayload().
Manually tested with Hadoop 3.1.2 and 3.3.0-SNAPSHOT (on top of HBASE-22103 and HBASE-23998 and set jetty.version=9.4.20)
Please let me know if this is the acceptable approach.