You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/05/17 02:31:10 UTC
[GitHub] [incubator-kvrocks] xiaobiaozhao opened a new pull request, #584: feat(ops): Add more compression
xiaobiaozhao opened a new pull request, #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584
## Description
Why need the PR?
Provides more Rocksdb.compression configuration parameters for customization
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] datavisorxiaobiaozhao commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
datavisorxiaobiaozhao commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r902604431
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
+
+message(STATUS "remove ${LZ4_SOURCE_DIR}")
+execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")
Review Comment:
Cool
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900882501
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
+
+message(STATUS "remove ${LZ4_SOURCE_DIR}")
+execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")
Review Comment:
I think these commands are not friendly to incremental build, could you consider to remove them?
##########
cmake/rocksdb.cmake:
##########
@@ -38,10 +38,14 @@ FetchContent_GetProperties(snappy)
FetchContent_MakeAvailableWithArgs(rocksdb
CMAKE_MODULE_PATH=${PROJECT_SOURCE_DIR}/cmake/modules # to locate FindJeMalloc.cmake
Snappy_DIR=${PROJECT_SOURCE_DIR}/cmake/modules # to locate SnappyConfig.cmake
+ lz4_ROOT_DIR=${LZ4_LIB_SOURCE_DIR}
+ lz4_LIBRARIES=${LZ4_LIB_SOURCE_DIR}/liblz4.a
+ lz4_INCLUDE_DIRS=${LZ4_LIB_SOURCE_DIR}
Review Comment:
Like cmake files in [cmake/modules](https://github.com/apache/incubator-kvrocks/tree/unstable/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.
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
Could you use [FetchContent](https://cmake.org/cmake/help/latest/module/FetchContent.html) instead of these manual downloading commands?
Just like [lua.cmake](https://github.com/apache/incubator-kvrocks/blob/unstable/cmake/lua.cmake#L22), it is easy to maintain and more straightforward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900949750
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
cool, thanks for Twice's explain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900940717
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
To facilitate the modification, I show a concrete example below:
```cmake
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 ...)
...
endif()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900927828
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1163154053
> 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:
```shell
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
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900936504
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
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 license issues than the manual way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900936504
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
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 than the manual way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r902726508
##########
cmake/rocksdb.cmake:
##########
@@ -38,10 +38,14 @@ FetchContent_GetProperties(snappy)
FetchContent_MakeAvailableWithArgs(rocksdb
CMAKE_MODULE_PATH=${PROJECT_SOURCE_DIR}/cmake/modules # to locate FindJeMalloc.cmake
Snappy_DIR=${PROJECT_SOURCE_DIR}/cmake/modules # to locate SnappyConfig.cmake
+ lz4_ROOT_DIR=${LZ4_LIB_SOURCE_DIR}
+ lz4_LIBRARIES=${LZ4_LIB_SOURCE_DIR}/liblz4.a
+ lz4_INCLUDE_DIRS=${LZ4_LIB_SOURCE_DIR}
Review Comment:
OK, I'll do these optimizations tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900882501
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
+
+message(STATUS "remove ${LZ4_SOURCE_DIR}")
+execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")
Review Comment:
I think these commands are not friendly to incremental build, could you consider to remove them?
We can let FetchContent to manipulate these file system stuff automatically.
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
+
+message(STATUS "remove ${LZ4_SOURCE_DIR}")
+execute_process(COMMAND sh -c "rm -rf ${LZ4_SOURCE_DIR}")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1164134882
> > @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1163886811
> @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](https://lists.apache.org/thread/0krk4nwwvzv9hgc10h6f3owo00qccsxo)). 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1163013676
The unit test failed in https://github.com/apache/incubator-kvrocks/runs/7003199495?check_suite_focus=true for 1ee66beb15407846cce3c9952dffd64ded2a3b98.
Just to record for more investigate, I will try to rerun the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r903655730
##########
.gitignore:
##########
@@ -44,4 +44,3 @@ Makefile.dep
make_config.mk
.vscode
kvrocks2redis
-build
Review Comment:
I think ignoring the `build` dir is useful, so could you keep it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1159415031
Do guys think this change makes sense for Kvrocks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900886147
##########
cmake/rocksdb.cmake:
##########
@@ -38,10 +38,14 @@ FetchContent_GetProperties(snappy)
FetchContent_MakeAvailableWithArgs(rocksdb
CMAKE_MODULE_PATH=${PROJECT_SOURCE_DIR}/cmake/modules # to locate FindJeMalloc.cmake
Snappy_DIR=${PROJECT_SOURCE_DIR}/cmake/modules # to locate SnappyConfig.cmake
+ lz4_ROOT_DIR=${LZ4_LIB_SOURCE_DIR}
+ lz4_LIBRARIES=${LZ4_LIB_SOURCE_DIR}/liblz4.a
+ lz4_INCLUDE_DIRS=${LZ4_LIB_SOURCE_DIR}
Review Comment:
Like cmake files in [cmake/modules](https://github.com/apache/incubator-kvrocks/tree/unstable/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 : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #584: Support to use lz4 compression in RocksDB
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1164288420
Thanks for @xiaobiaozhao contribution again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1164019349
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#issuecomment-1128339236
The biggest problem we need to solve is how to compile these compression algorithms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r902708795
##########
.gitignore:
##########
@@ -44,4 +44,10 @@ Makefile.dep
make_config.mk
.vscode
kvrocks2redis
+<<<<<<< HEAD
build
+=======
+
+build/
+external/
+>>>>>>> feat: add lz4.cmake
Review Comment:
could you try to resolve the conflict?
##########
cmake/rocksdb.cmake:
##########
@@ -38,10 +38,14 @@ FetchContent_GetProperties(snappy)
FetchContent_MakeAvailableWithArgs(rocksdb
CMAKE_MODULE_PATH=${PROJECT_SOURCE_DIR}/cmake/modules # to locate FindJeMalloc.cmake
Snappy_DIR=${PROJECT_SOURCE_DIR}/cmake/modules # to locate SnappyConfig.cmake
+ lz4_ROOT_DIR=${LZ4_LIB_SOURCE_DIR}
+ lz4_LIBRARIES=${LZ4_LIB_SOURCE_DIR}/liblz4.a
+ lz4_INCLUDE_DIRS=${LZ4_LIB_SOURCE_DIR}
Review Comment:
maybe you should replace these to dir in `${lz4_SOURCE_DIR}/lib`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900936504
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r900940717
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
To facilitate the modification, I show a concrete example below:
```cmake
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 make ...)
...
endif()
```
##########
cmake/lz4.cmake:
##########
@@ -0,0 +1,82 @@
+# 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.
+
+include_guard()
+
+# lib name
+set(LZ4_LIB_NAME "lz4")
+# set lz4
+set(LZ4_SOURCE "lz4-1.9.3")
+# set download dir
+set(LZ4_ZIP_DL_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps)
+# set zip file name
+set(LZ4_ZIP_NAME "lz4-1.9.3.tar.gz")
+#
+set(LZ4_ZIP_ABSOLUT_PATH "${LZ4_ZIP_DL_DIR}/lz4-1.9.3.tar.gz")
+# url
+set(LZ4_DOWNLOAD_URL "https://github.com/lz4/lz4/archive/v1.9.3.tar.gz")
+# md5sum
+set(LZ4_MD5SUM "3a1ab1684e14fc1afc66228ce61b2db3")
+# set source dir
+set(LZ4_SOURCE_DIR "${LZ4_ZIP_DL_DIR}/${LZ4_SOURCE}")
+# include dir
+set(LZ4_LIB_INCLUDE_DIR ${LZ4_SOURCE_DIR}/lib)
+# src dir
+set(LZ4_LIB_SOURCE_DIR ${LZ4_SOURCE_DIR}/lib)
+# download timeout
+set(DOWNLOAD_LZ4_TIMEOUT 600 CACHE STRING "Timeout in seconds when downloading LZ4.")
+
+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()
Review Comment:
To facilitate the modification, I show a concrete example below:
```cmake
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()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #584: feat(ops): Add more compression
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #584:
URL: https://github.com/apache/incubator-kvrocks/pull/584#discussion_r902715206
##########
.gitignore:
##########
@@ -44,4 +44,10 @@ Makefile.dep
make_config.mk
.vscode
kvrocks2redis
+<<<<<<< HEAD
build
+=======
+
+build/
+external/
+>>>>>>> feat: add lz4.cmake
Review Comment:
I think just keeping the HEAD is fine, `external` is useless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org