HADOOP-17125. Using snappy-java in SnappyCodec #2201
Closed
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
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
thanks for the proposal and thanks for the PR |
Member
Author
|
@hemanthboyina The performance should be similar since the java-snappy also uses native lib that is bundled in the jar file. I don't benchmark yet, and I will try to find time to do it. If possible, feel free to do the testing and post the result back. |
Contributor
|
ideally there shouldn't be any difference in performance , better we have a benchmark result of native snappy and java-snappy |
|
💔 -1 overall
This message was automatically generated. |
Member
|
💔 -1 overall
This message was automatically generated. |
Member
Author
|
@sunchao thanks! That helps a lot. I rebased on trunk, and modified the script to remove snappy native build. Let's see how it works. |
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
Member
|
Thanks for working on this, @dbtsai. Then patch breaks build if snappy(-devel) is installed on the env. hadoop-common-project/hadoop-common/src/CMakeLists.txt must be updated to address this. |
Member
|
Removing the snappy related code from hadoop-mapreduce-client-nativetask breaks existing deployment using the feature? Since imcompatible change is hard to get consensus and takes time to come in, how about fixing only hadoop-common first and keeping hadoop-mapreduce-client-nativetask as is here? BUILDING.txt should refer snappy-dev(el) as optional dependency only used for hadoop-mapreduce-client-nativetask in that case. |
Member
Author
|
@iwasakims Good point. I'll revert the |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
viirya
reviewed
Sep 8, 2020
| compressedDirectBuf.position(0).limit(compressedDirectBufLen); | ||
| // There is compressed input, decompress it now. | ||
| int size = Snappy.uncompressedLength((ByteBuffer) compressedDirectBuf); | ||
| if (size > uncompressedDirectBuf.capacity()) { |
Member
There was a problem hiding this comment.
Should we check with uncompressedDirectBuf.remaining instead of capacity?
Member
Author
There was a problem hiding this comment.
Before calling decompressBytesDirect, we always reset uncompressedDirectBuf. So both method returns the same result. But you are right, I think .remaining is better.
Member
There was a problem hiding this comment.
decompressDirect also calls decompressBytesDirect, but the uncompressedDirectBuf is passed in as an argument. I think it has danger to assume uncompressedDirectBuf is always reset.
| return 0; | ||
| } else { | ||
| // Set the position and limit of `compressedDirectBuf` for reading | ||
| compressedDirectBuf.position(0).limit(compressedDirectBufLen); |
Member
There was a problem hiding this comment.
I'm not sure we need to set position and limit here? If compressedDirectBuf already has been set with position and limit before calling decompressBytesDirect? Won't we read wrong data from this buffer?
Member
Author
There was a problem hiding this comment.
decompressBytesDirect is the only call that reads the data from compressedDirectBuf, so I think we should read it from the beginning. In compressedDirectBuf, we should fully decompress the compressedDirectBuf.
Member
There was a problem hiding this comment.
For decompressDirect, compressedDirectBuf is set up correctly before calling decompressBytesDirect. So we don't need to do it here again. For decompress, I'm not sure, but looks like setInputFromSavedData also takes care about compressedDirectBuf reseting. I guess we don't need to do it in decompressBytesDirect.
Member
Author
|
Closing this PR in favor of #2297 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
See https://issues.apache.org/jira/browse/HADOOP-17125 for details