Skip to content
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.

Sign up for GitHub

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 22 commits into from
Oct 10, 2018
+610 −86
Merged

KAFKA-4514: Add Codec for ZStandard Compression #2267

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d4b2bf4
KAFKA-4514: Add Codec for ZStandard Compression
dongjinleekr Dec 16, 2016
bda05f7
Minor updates
dongjinleekr Mar 10, 2018
82ba81a
Update ZStd version: 1.3.3 -> 1.3.4 (zstd-jni 1.3.4-10)
dongjinleekr Jun 13, 2018
51bdf15
Update ZStd version: 1.3.4 -> 1.3.5 (zstd-jni 1.3.5-2)
dongjinleekr Jul 14, 2018
35dcffa
Add license description of zstd and zstd-jni
dongjinleekr Aug 12, 2018
8872bdf
Disallow fetch request with API version < 10 for ZStandard compressed…
dongjinleekr Aug 31, 2018
fbdcabe
Disallow produce request with API version < 7 for ZStandard compresse…
dongjinleekr Sep 16, 2018
74d6b19
Disallow MemoryRecordsBuilder instantiation with ZSTD compression cod…
dongjinleekr Sep 21, 2018
671e12f
Disallow downconversion of ZSTD compressed records
dongjinleekr Sep 25, 2018
c6bbabd
Make the constructor of ProduceRequest.Builder public: 1. parity with…
dongjinleekr Sep 26, 2018
e5a7fd4
Add integration tests: FetchRequestTest, ProduceRequestTest
dongjinleekr Sep 26, 2018
06b2a59
Update zstd-jni version: 1.3.5-2 -> 1.3.5-4
dongjinleekr Sep 27, 2018
561d21c
Fix: remove unneeded newline addition at CompressionType.java
dongjinleekr Sep 28, 2018
3b4f6ae
Revert "Disallow downconversion of ZSTD compressed records"
dongjinleekr Oct 3, 2018
607defc
Reimplement down-conversion logic
dongjinleekr Oct 3, 2018
f34d323
Update for the previous commit (Thanks to Jason Gustafson's guide)
dongjinleekr Oct 4, 2018
1eee287
Add UNSUPPORTED_COMPRESSION_TYPE to the possible error codes list of …
dongjinleekr Oct 5, 2018
bf95957
Refactor tests: MemoryRecordsTest, MemoryRecordsBuilderTest
dongjinleekr Oct 5, 2018
36d77af
Remove ProduceRequest version check in KafkaApis#handleProduceRequest…
dongjinleekr Oct 5, 2018
54eaa61
Add validation for the case magic < 2 with Zstandard Compression
dongjinleekr Oct 5, 2018
65c57a1
Consistency: all log or exception messages are using 'ZStandard', not…
dongjinleekr Oct 5, 2018
147c473
Add validation for ProduceRequest: update ProduceRequest.validateRecords
dongjinleekr Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add license description of zstd and zstd-jni
@dongjinleekr @hachikuji
dongjinleekr authored and hachikuji committed Oct 9, 2018
commit 35dcffa5d1c391f74913efc894c73019261f1f7e
66 changes: 66 additions & 0 deletions LICENSE
View file
Original file line number Diff line number Diff line change
@@ -201,6 +201,7 @@
See the License for the specific language governing permissions and
limitations under the License.

------------------------------------------------------------------------------------
This distribution has a binary dependency on jersey, which is available under the CDDL
License as described below.

@@ -328,3 +329,68 @@ As between Initial Developer and the Contributors, each party is responsible for
NOTICE PURSUANT TO SECTION 9 OF THE COMMON DEVELOPMENT AND DISTRIBUTION LICENSE (CDDL)

The code released under the CDDL shall be governed by the laws of the State of California (excluding conflict-of-law provisions). Any litigation relating to this License shall be subject to the jurisdiction of the Federal Courts of the Northern District of California and the state courts of the State of California, with venue lying in Santa Clara County, California.

------------------------------------------------------------------------------------
This distribution has a binary dependency on zstd, which is available under the BSD 3-Clause License as described below.

BSD License

For Zstandard software

Copyright (c) 2016-present, Facebook, Inc. All rights reserved.

Redistribution and use in source and binary forms, with or without modification,
are permitted provided that the following conditions are met:

* Redistributions of source code must retain the above copyright notice, this
list of conditions and the following disclaimer.

* Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.

* Neither the name Facebook nor the names of its contributors may be used to
endorse or promote products derived from this software without specific
prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

------------------------------------------------------------------------------------
This distribution has a binary dependency on zstd-jni, which is available under the BSD 2-Clause License
as described below.

Zstd-jni: JNI bindings to Zstd Library

Copyright (c) 2015-2016, Luben Karavelov/ All rights reserved.

BSD License

Redistribution and use in source and binary forms, with or without modification,
are permitted provided that the following conditions are met:

* Redistributions of source code must retain the above copyright notice, this
list of conditions and the following disclaimer.

* Redistributions in binary form must reproduce the above copyright notice, this
list of conditions and the following disclaimer in the documentation and/or
other materials provided with the distribution.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Copy link
Member

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

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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.