WIP. HADOOP-17124. Support LZO Codec using aircompressor #3612
Conversation
This file contains 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
|
|
viirya
commented
Nov 3, 2021
Comment on lines
384
to
388
| <groupId>com.hadoop.gplcompression</groupId> | ||
| <artifactId>hadoop-lzo</artifactId> | ||
| <version>0.4.21-SNAPSHOT</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Put it here for verifying the new Lzo codec only. We will remove it before merging.
viirya
commented
Nov 3, 2021
| /** | ||
| * This class creates lzo compressors/decompressors. | ||
| */ | ||
| public class LzoCodec2 implements Configurable, CompressionCodec { |
There was a problem hiding this comment.
We will rename to LzoCodec before merging.
|
💔 -1 overall
This message was automatically generated. |
viirya
commented
Nov 3, 2021
Comment on lines
+2466
to
+2471
| <repository> | ||
| <id>twitter</id> | ||
| <url>https://maven.twttr.com/</url> | ||
| </repository> |
There was a problem hiding this comment.
We will remove this before merging.
|
Comparing with #2159, this adds |
|
💔 -1 overall
This message was automatically generated. |
|
Hmm, for the test, I built |
|
💔 -1 overall
This message was automatically generated. |
|
Hmm, seems not work. I checked Maybe the CI is not the target? So seems we cannot run the comparison test between the GPL lzo codec and this lzo codec on Hadoop CI. I have run them locally to verify the comparison. The reviewers may build and install GPL lzo locally to run the test. Once the reviewers think it is okay, I will remove GPL lzo codec stuffs. |
|
Thanks @viirya , I'll take a look soon. |
|
@viirya can you share how to test this locally? I got the same error as above and curious why it didn't work even |
|
@sunchao It doesn't include native library for Mac OS X. So we need to built it. I built and installed this https://github.com/twitter/hadoop-lzo locally. You may need to revert e7d98ab to use |
|
Cool thanks. I verified locally and the relevant tests in |
|
Thanks @sunchao . So I think we verified the new Lzo compressor and GPL Lzo compressor. |
|
Yes looks OK so far. For reference, in the PR description could you add what other projects are using Feel free to update the PR and mark it as ready for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
This patch adds LZO Codec from
aircompressorwhich includesLzoCompressorandLzoDecompressor(WIP).See https://issues.apache.org/jira/browse/HADOOP-17124 for details.
The mostly famous usage of
aircompressoris trino. Trino uses the library for its Lz4Codec, LzoCodec, SnappyCodec, etc. The code link is:https://github.com/trinodb/trino/blob/fe608f2723842037ff620d612a706900e79c52c8/lib/trino-rcfile/src/main/java/io/trino/rcfile/AircompressorCodecFactory.java
How was this patch tested?
Unit test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?