-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
WIP. HADOOP-17124. Support LZO Codec using aircompressor #3612
base: trunk
Are you sure you want to change the base?
Conversation
|
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put it here for verifying the new Lzo codec only. We will remove it before merging.
/** | ||
* This class creates lzo compressors/decompressors. | ||
*/ | ||
public class LzoCodec2 implements Configurable, CompressionCodec { |
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.
We will rename to LzoCodec
before merging.
💔 -1 overall
This message was automatically generated. |
<repository> | ||
<id>twitter</id> | ||
<url>https://maven.twttr.com/</url> | ||
</repository> |
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.
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. |
Description of PR
This patch adds LZO Codec from
aircompressor
which includesLzoCompressor
andLzoDecompressor
(WIP).See https://issues.apache.org/jira/browse/HADOOP-17124 for details.
The mostly famous usage of
aircompressor
is 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-binary
files?