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

compressor: add LZ4 support #15434

Merged
merged 2 commits into from Jun 7, 2017

Conversation

Projects
None yet
5 participants
@yuyuyu101
Member

yuyuyu101 commented Jun 2, 2017

No description provided.

class LZ4Compressor : public Compressor {
public:
LZ4Compressor() : Compressor(COMP_ALG_LZ4, "snappy") {}

This comment has been minimized.

@tchaikov

tchaikov Jun 2, 2017

Contributor

s/snappy/lz4/

bufferlist &dst) override {
uint32_t count;
::decode(count, p);
uint32_t origin_lens[count];

This comment has been minimized.

@tchaikov

tchaikov Jun 2, 2017

Contributor

we have -Wvla enabled now. so...

if(WITH_EMBEDDED)
include(MergeStaticLibraries)
add_library(cephd_compressor_base STATIC ${compressor_srcs})
set_target_properties(cephd_compressor_base PROPERTIES COMPILE_DEFINITIONS BUILDING_FOR_EMBEDDED)
merge_static_libraries(cephd_compressor cephd_compressor_base cephd_compressor_snappy cephd_compressor_zlib cephd_compressor_zstd)
merge_static_libraries(cephd_compressor cephd_compressor_base

This comment has been minimized.

@tchaikov

tchaikov Jun 2, 2017

Contributor

could you take this opportunity to reformat the merged libraries? so we have one of them each line.

@@ -11,15 +11,18 @@ set(compressor_plugin_dir ${CMAKE_INSTALL_PKGLIBDIR}/compressor)
add_subdirectory(snappy)
add_subdirectory(zlib)
add_subdirectory(zstd)
add_subdirectory(lz4)

This comment has been minimized.

@tchaikov

tchaikov Jun 2, 2017

Contributor

shall we conditionalize it like

if (LZ4_FOUND)
  add_subdirectory(lz4)
endif()

?

@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Jun 3, 2017

done

@@ -12,14 +12,33 @@ add_subdirectory(snappy)
add_subdirectory(zlib)
add_subdirectory(zstd)
add_custom_target(compressor_plugins DEPENDS
if (LZ4_FOUND)

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

is lz4 plugin optional or not?

This comment has been minimized.

@yuyuyu101

yuyuyu101 Jun 3, 2017

Member

optional

@@ -12,14 +12,33 @@ add_subdirectory(snappy)
add_subdirectory(zlib)
add_subdirectory(zstd)
add_custom_target(compressor_plugins DEPENDS
if (LZ4_FOUND)
add_subdirectory(lz4)

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

indent.

ceph_snappy
ceph_zlib
ceph_zstd)
if (LZ4_FOUND)
list(APPEND ceph_compressor_libs ceph_lz4)

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

indent.

add_dependencies(ceph_lz4 ${CMAKE_SOURCE_DIR}/src/ceph_ver.h)
target_link_libraries(ceph_lz4 ${LZ4_LIBRARY})
set_target_properties(ceph_lz4 PROPERTIES
VERSION 2.0.0

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

why the soversion starts at 2.0? and i am wondering if we need to version plugin at all?

This comment has been minimized.

@yuyuyu101

yuyuyu101 Jun 3, 2017

Member

I'm not sure, just copy from other

@@ -322,6 +322,7 @@ INSTANTIATE_TEST_CASE_P(
Compressor,
CompressorTest,
::testing::Values(
"lz4",

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

should be conditionalized if lz4 compressor plugin is optional.

@@ -257,6 +257,7 @@ if(NOT ${ATOMIC_OPS_FOUND})
endif(NOT ${ATOMIC_OPS_FOUND})
find_package(snappy REQUIRED)
find_package(LZ4 REQUIRED)

This comment has been minimized.

@tchaikov

tchaikov Jun 3, 2017

Contributor

if lz4 is optional, then it's not REQUIRED.

yuyuyu101 added some commits Jun 2, 2017

compressor: add LZ4 support
Signed-off-by: Haomai Wang <haomai@xsky.com>
test_compression: add lz4
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Jun 3, 2017

@tchaikov done

@liewegas liewegas merged commit b34507f into ceph:master Jun 7, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@yuyuyu101 yuyuyu101 deleted the yuyuyu101:wip-lz4 branch Jun 7, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 14, 2017

@yuyuyu101 thanks for this!

is there a reason we don't enable this by default? the rgw suite is using the random compression type, so it tries to use it even though it's disabled. this doesn't cause any test failures, but does produce a lot of these warnings:

load failed dlopen(): "/usr/lib64/ceph/compressor/libceph_lz4.so: cannot open shared object file: No such file or directory"

my preference would be to enable it by default and require the lz4 dependency. alternatively, we could make the COMP_ALG_LZ4 enum conditional on HAVE_LZ4 so it wouldn't be selected by std::uniform_int_distribution<> dist(0, COMP_ALG_LAST - 1);

(cc @pritha-srivastava @aclamk)

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 14, 2017

@Firefishy

This comment has been minimized.

Contributor

Firefishy commented Aug 12, 2017

@yuyuyu101 Any reason why WITH_LZ4 wasn't enabled by default?

@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Aug 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment