-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
KAFKA-4514: Add Codec for ZStandard Compression #2267
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
d4b2bf4
KAFKA-4514: Add Codec for ZStandard Compression
dongjinleekr bda05f7
Minor updates
dongjinleekr 82ba81a
Update ZStd version: 1.3.3 -> 1.3.4 (zstd-jni 1.3.4-10)
dongjinleekr 51bdf15
Update ZStd version: 1.3.4 -> 1.3.5 (zstd-jni 1.3.5-2)
dongjinleekr 35dcffa
Add license description of zstd and zstd-jni
dongjinleekr 8872bdf
Disallow fetch request with API version < 10 for ZStandard compressed…
dongjinleekr fbdcabe
Disallow produce request with API version < 7 for ZStandard compresse…
dongjinleekr 74d6b19
Disallow MemoryRecordsBuilder instantiation with ZSTD compression cod…
dongjinleekr 671e12f
Disallow downconversion of ZSTD compressed records
dongjinleekr c6bbabd
Make the constructor of ProduceRequest.Builder public: 1. parity with…
dongjinleekr e5a7fd4
Add integration tests: FetchRequestTest, ProduceRequestTest
dongjinleekr 06b2a59
Update zstd-jni version: 1.3.5-2 -> 1.3.5-4
dongjinleekr 561d21c
Fix: remove unneeded newline addition at CompressionType.java
dongjinleekr 3b4f6ae
Revert "Disallow downconversion of ZSTD compressed records"
dongjinleekr 607defc
Reimplement down-conversion logic
dongjinleekr f34d323
Update for the previous commit (Thanks to Jason Gustafson's guide)
dongjinleekr 1eee287
Add UNSUPPORTED_COMPRESSION_TYPE to the possible error codes list of …
dongjinleekr bf95957
Refactor tests: MemoryRecordsTest, MemoryRecordsBuilderTest
dongjinleekr 36d77af
Remove ProduceRequest version check in KafkaApis#handleProduceRequest…
dongjinleekr 54eaa61
Add validation for the case magic < 2 with Zstandard Compression
dongjinleekr 65c57a1
Consistency: all log or exception messages are using 'ZStandard', not…
dongjinleekr 147c473
Add validation for ProduceRequest: update ProduceRequest.validateRecords
dongjinleekr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
34 changes: 34 additions & 0 deletions
34
...nts/src/main/java/org/apache/kafka/common/errors/UnsupportedCompressionTypeException.java
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.apache.kafka.common.errors; | ||
|
||
/** | ||
* The requesting client does not support the compression type of given partition. | ||
*/ | ||
public class UnsupportedCompressionTypeException extends ApiException { | ||
|
||
private static final long serialVersionUID = 1L; | ||
|
||
public UnsupportedCompressionTypeException(String message) { | ||
super(message); | ||
} | ||
|
||
public UnsupportedCompressionTypeException(String message, Throwable cause) { | ||
super(message, cause); | ||
} | ||
|
||
} |
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
Is this LICENSE change needed? Cc @ewencp
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'm not sure why most of the additions here were made, tbh.
All of this with the caveat IANAL and any questions re: licensing are best directed at ASF legal.
In many cases, the NOTICE file might be a more appropriate location depending on where the LICENSE actually gets pulled into the binary distribution. This gets confusing because of handling both source and binary distributions. In source distributions, including additional info in the LICENSE file would really only apply to anything copied in afaik, since the source distribution doesn't include any of the dependency binaries, i.e. what's under license in that distribution is only our code. (Probably gets messier with top-of-file copyright notices..., and even NOTICE files seem messy because, e.g., Apache explicitly calls them out while other licenses don't)
In binary distributions, adding here wouldn't change, for example, whether we reproduce the BSD license notice as requested -- either you think it needs to be directly printed (which this doesn't do) or just including the original JARs with their original licenses would be enough since any binary distribution that includes the dependencies would have that in the jar (or if they were missing, upstream should fix that). At a bare minimum, we don't currently include nearly all the dependency licenses here and I don't think most Apache projects do. I haven't looked back at the history to understand what triggered people to start including more than the Apache license in this file.
Again, IANAL, but it's actually unclear you really need anything beyond the raw license given the way source distributions and java binary distributions would work & preserve other contents (modulo uberjar conflicts, which we don't do). I think Apache is the only OSS license where there's a "readable" phrase that might complicate things.
But IANAL, so probably figuring this out w/ apache legal would give us more confidence in the answer.
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.
@ijuma @ewencp The spark folks appear to have taken a principled approach here: apache/spark#21640. Ultimately they still include the zstd and zstd-jni license files. I think we can probably commit this as is and perhaps file a JIRA to come up with a standardized approach (probably just use the same convention as spark). Does that sound reasonable?
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.
FYI: In fact, apache/spark#21640 (what @hachikuji pointed out) is what I referred to while I was working on this commit. (It is also mentioned in the KIP document.) It was initiated from this message in spark-dev mailing list, sent by a PMC member.
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.
@hachikuji I'm fine with merging and submitting a JIRA. It's clear that we're not consistent, so we probably should clean it up before the release.
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.
Sounds good. I will merge today.