Support to use lz4 compression in RocksDB #584

xiaobiaozhao · 2022-05-17T02:31:08Z

Description

Why need the PR?
Provides more Rocksdb.compression configuration parameters for customization

This closes #601

ShooterIT · 2022-05-17T02:37:56Z

The biggest problem we need to solve is how to compile these compression algorithms.

git-hulk · 2022-06-18T09:57:42Z

Do guys think this change makes sense for Kvrocks?

PragmaTwice

Great job! Thanks for your hard work. Comments inline.

PragmaTwice · 2022-06-18T12:47:01Z

cmake/lz4.cmake

+if (EXISTS ${LZ4_ZIP_ABSOLUT_PATH})
+	file(MD5 ${LZ4_ZIP_ABSOLUT_PATH} LZ4_MD5SUM_)
+	if (NOT ${LZ4_MD5SUM} STREQUAL ${LZ4_MD5SUM_})
+		message(STATUS "remove -f ${LZ4_ZIP_ABSOLUT_PATH}")
+		execute_process(COMMAND sh -c "rm -f ${LZ4_ZIP_ABSOLUT_PATH}")
+	endif()
+endif()
+
+if (NOT EXISTS ${LZ4_ZIP_ABSOLUT_PATH})
+	# download
+	message(STATUS "Downloading ${LZ4_ZIP_NAME} to ${LZ4_ZIP_ABSOLUT_PATH}")
+	file(DOWNLOAD ${LZ4_DOWNLOAD_URL}
+		${LZ4_ZIP_ABSOLUT_PATH}
+		TIMEOUT ${DOWNLOAD_LZ4_TIMEOUT}
+		STATUS ERR SHOW_PROGRESS)
+endif()
+
+if (EXISTS ${LZ4_ZIP_ABSOLUT_PATH})
+	file(MD5 ${LZ4_ZIP_ABSOLUT_PATH} LZ4_MD5SUM_)
+	if (NOT ${LZ4_MD5SUM} STREQUAL ${LZ4_MD5SUM_})
+		message(FATAL_ERROR "${LZ4_ZIP_ABSOLUT_PATH} seems be something wrong, please check")
+	endif()
+else()
+	message(FATAL_ERROR "${LZ4_ZIP_ABSOLUT_PATH} seems be something wrong, please check")
+endif()


Could you use FetchContent instead of these manual downloading commands?
Just like lua.cmake, it is easy to maintain and more straightforward.

Lz4 dirs are under GPL license except the lib which we are needed, so NOT sure whether is it ok to use FetchContent like others.

We can just fetch files and do not call add_subdirectory (do not use these cmake files in lz4), like in lua.cmake, so I think it has few difference on the license issue than the manual way.

To facilitate the modification, I show a concrete example below:

FetchContent_Declare(lz4 URL https://github.com/lz4/lz4/archive/v1.9.3.tar.gz ) FetchContent_GetProperties(lz4) if(NOT lz4_POPULATED) FetchContent_Populate(lz4) add_custom_target(make_lz4 COMMAND make ...) ... endif()

cool, thanks for Twice's explain.

PragmaTwice · 2022-06-18T12:51:50Z

cmake/lz4.cmake

+message(STATUS "remove ${LZ4_SOURCE_DIR}")
+execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")


I think these commands are not friendly to incremental build, could you consider to remove them?
We can use FetchContent to manipulate these file system stuff automatically.

PragmaTwice · 2022-06-18T12:58:42Z

cmake/rocksdb.cmake

+  lz4_ROOT_DIR=${LZ4_LIB_SOURCE_DIR}
+  lz4_LIBRARIES=${LZ4_LIB_SOURCE_DIR}/liblz4.a
+  lz4_INCLUDE_DIRS=${LZ4_LIB_SOURCE_DIR}


Like cmake files in cmake/modules, injecting the lz4 target via hooking find package mechanism can propagate dependency relation into the rocksdb target, in addition this makes the dependencies more modular, we can easily do some switches to selectively enable them.
Of course, if you are not familiar with them, I can follow up to help you implement them : )

1. make lz4.cmake more cmake style

.gitignore

cmake/rocksdb.cmake

.gitignore

PragmaTwice · 2022-06-22T12:04:03Z

The unit test failed in https://github.com/apache/incubator-kvrocks/runs/7003199495?check_suite_focus=true for 1ee66be.
Just to record for more investigate, I will try to rerun the CI.

PragmaTwice

LGTM. cc @git-hulk @tisonkun @ShooterIT

tisonkun

Cool! Is there any test coverage for lz4 compression option? Or do you verify it locally?

git-hulk · 2022-06-22T14:11:35Z

Cool! Is there any test coverage for lz4 compression option? Or do you verify it locally?

I tested it on my side, run with LZ4 is ok and check the symbols:

000000000072a890 t LZ4HC_compress_generic_dictCtx
0000000000725ec0 t LZ4HC_compress_generic_noDictCtx.part.0
00000000007216f0 t LZ4HC_compress_optimal
0000000000730790 T LZ4_attach_HC_dictionary
0000000000714790 T LZ4_attach_dictionary
0000000000721590 T LZ4_compress
000000000070b9a0 T LZ4_compressBound
0000000000730be0 T LZ4_compressHC
0000000000730ec0 T LZ4_compressHC2
00000000007319f0 T LZ4_compressHC2_continue
...

tisonkun

@git-hulk thanks for your input. +1 to merge.

BTW do you confirm that introduce lz4 support in this way won't cause dependency issue? IIRC lz4 can contain GPLv2 code, but if we use lib only, it should be fine.

git-hulk · 2022-06-23T03:34:17Z

@git-hulk thanks for your input. +1 to merge.

BTW do you confirm that introduce lz4 support in this way won't cause dependency issue? IIRC lz4 can contain GPLv2 code, but if we use lib only, it should be fine.

Yes that we use lib only, but NOT sure about whether we need to involve more people to discuss or not(previous discussion thread). I saw some apache projects also used the LZ4 with downloading the repo: https://github.com/search?l=CMake&q=org%3Aapache+lz4%2Flz4&type=Code.

PragmaTwice · 2022-06-23T06:48:50Z

@git-hulk thanks for your input. +1 to merge.

BTW do you confirm that introduce lz4 support in this way won't cause dependency issue? IIRC lz4 can contain GPLv2 code, but if we use lib only, it should be fine.

Yeah, I think this way only uses files in the lib directory, so there will be no GPL issues involved.

git-hulk · 2022-06-23T08:51:15Z

@git-hulk thanks for your input. +1 to merge.
BTW do you confirm that introduce lz4 support in this way won't cause dependency issue? IIRC lz4 can contain GPLv2 code, but if we use lib only, it should be fine.

Yeah, I think this way only uses files in the lib directory, so there will be no GPL issues involved.

So, let's merge this PR if this's crystal clear. cc @tisonkun @PragmaTwice @ShooterIT

git-hulk · 2022-06-23T11:22:40Z

Thanks for @xiaobiaozhao contribution again!

xiaobiaozhao force-pushed the add_compression branch 2 times, most recently from 5d5e108 to f1d605c Compare June 8, 2022 01:30

git-hulk requested review from PragmaTwice, ShooterIT and tisonkun June 18, 2022 09:57

PragmaTwice reviewed Jun 18, 2022

View reviewed changes

xiaobiaozhao added 6 commits June 21, 2022 22:19

✨ feat: add more comp opts

39c8324

✨ feat: update doc

3a2742f

feat: add lz4.cmake

a463e68

✨ feat: rocksdb add lz4

19c86cf

✨ feat: update cmake

91bbb4e

🦄 refactor: update lz4.cmake

432a1b5

1. make lz4.cmake more cmake style

xiaobiaozhao force-pushed the add_compression branch from 59afeaf to 432a1b5 Compare June 21, 2022 14:23

PragmaTwice reviewed Jun 21, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

cmake/rocksdb.cmake Outdated Show resolved Hide resolved

🐞 fix: update cmake

1ee66be

PragmaTwice reviewed Jun 22, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

: resolve .gitignore

98bcbbd

PragmaTwice approved these changes Jun 22, 2022

View reviewed changes

PragmaTwice requested a review from git-hulk June 22, 2022 13:31

Merge branch 'unstable' into add_compression

50331b8

tisonkun reviewed Jun 22, 2022

View reviewed changes

git-hulk approved these changes Jun 22, 2022

View reviewed changes

tisonkun approved these changes Jun 22, 2022

View reviewed changes

Merge branch 'unstable' into add_compression

be28a08

git-hulk merged commit d42f029 into apache:unstable Jun 23, 2022

git-hulk changed the title ~~feat(ops): Add more compression~~ Support to use lz4 compression in RocksDB Jun 23, 2022

git-hulk added enhancement type enhancement release notes labels Jun 23, 2022

PragmaTwice mentioned this pull request Jun 23, 2022

Fix lz4 in CMake: support ninja & add Findlz4.cmake #654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to use lz4 compression in RocksDB #584

Support to use lz4 compression in RocksDB #584

xiaobiaozhao commented May 17, 2022 •

edited by git-hulk

Loading

ShooterIT commented May 17, 2022

git-hulk commented Jun 18, 2022

PragmaTwice left a comment

PragmaTwice Jun 18, 2022

git-hulk Jun 18, 2022

PragmaTwice Jun 18, 2022 •

edited

Loading

PragmaTwice Jun 18, 2022 •

edited

Loading

git-hulk Jun 18, 2022

PragmaTwice Jun 18, 2022 •

edited

Loading

datavisorxiaobiaozhao Jun 21, 2022

PragmaTwice Jun 18, 2022 •

edited

Loading

PragmaTwice commented Jun 22, 2022

PragmaTwice left a comment

tisonkun left a comment

git-hulk commented Jun 22, 2022

tisonkun left a comment

git-hulk commented Jun 23, 2022

PragmaTwice commented Jun 23, 2022

git-hulk commented Jun 23, 2022

git-hulk commented Jun 23, 2022

		message(STATUS "remove ${LZ4_SOURCE_DIR}")
		execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")

Support to use lz4 compression in RocksDB #584

Support to use lz4 compression in RocksDB #584

Conversation

xiaobiaozhao commented May 17, 2022 • edited by git-hulk Loading

Description

ShooterIT commented May 17, 2022

git-hulk commented Jun 18, 2022

PragmaTwice left a comment

Choose a reason for hiding this comment

PragmaTwice Jun 18, 2022

Choose a reason for hiding this comment

git-hulk Jun 18, 2022

Choose a reason for hiding this comment

PragmaTwice Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

PragmaTwice Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

git-hulk Jun 18, 2022

Choose a reason for hiding this comment

PragmaTwice Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

datavisorxiaobiaozhao Jun 21, 2022

Choose a reason for hiding this comment

PragmaTwice Jun 18, 2022 • edited Loading

Choose a reason for hiding this comment

PragmaTwice commented Jun 22, 2022

PragmaTwice left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

git-hulk commented Jun 22, 2022

tisonkun left a comment

Choose a reason for hiding this comment

git-hulk commented Jun 23, 2022

PragmaTwice commented Jun 23, 2022

git-hulk commented Jun 23, 2022

git-hulk commented Jun 23, 2022

xiaobiaozhao commented May 17, 2022 •

edited by git-hulk

Loading

PragmaTwice Jun 18, 2022 •

edited

Loading

PragmaTwice Jun 18, 2022 •

edited

Loading

PragmaTwice Jun 18, 2022 •

edited

Loading

PragmaTwice Jun 18, 2022 •

edited

Loading