-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-27668 PB's parseDelimitedFrom can successfully return when ther… #5059
Conversation
🎊 +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. |
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.
The idea is good, will take more detailed look.
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
try { | ||
return parser.parseFrom(ByteStreams.limit(in, size)); | ||
} catch (InvalidProtocolBufferException e) { | ||
throw e.unwrapIOException(); |
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.
Shall we also log InvalidProtocolBufferException
before throwing here?
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 piece of code is just copied from the protobuf code base, I think it is OK to not logging here?
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.
I see, no worries
// This is very useful for saving the extra effort for reconstructing the compression | ||
// dictionary, as DFSInputStream implement the available method, so in most cases we will | ||
// not reach here if there are not enough data. | ||
resetCompression = true; |
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.
Interesting. So this was introduced by HBASE-27621 and looks like has not made it to live release yet, correct?
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.
I just reversed the condition judgement here, no actual logic change. In HBASE-27621, it defaults to true and we set it to false when parsing WALKey fails. And when implementing HBASE-27632, I found a better way is to set it default to false, and when we begin to parse WALEdit, i.e, Cells, we set it to true.
💔 -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. |
Any other concerns? @virajjasani Thanks. |
💔 -1 overall
This message was automatically generated. |
…e are not enough bytes
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Left one nit, +1 otherwise
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/ProtobufLogReader.java
Outdated
Show resolved
Hide resolved
…e are not enough bytes (#5059) Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit d1fede7)
…e are not enough bytes (#5059) Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit d1fede7)
…e are not enough bytes (#5059) Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit d1fede7)
…e are not enough bytes (apache#5059) Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit d1fede7) (cherry picked from commit 890c89b) Change-Id: Ifeccab2a2a3ccae61b46f28928e340892837a71a
…e are not enough bytes