You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/06/26 06:05:00 UTC

[GitHub] [hbase-native-client] bharathv commented on a change in pull request #6: HBASE-23105: Download lib double conversion, fizz, update folly

bharathv commented on a change in pull request #6:
URL: https://github.com/apache/hbase-native-client/pull/6#discussion_r445974129



##########
File path: CMakeLists.txt
##########
@@ -117,23 +124,22 @@ endif()
 
 include_directories("${JAVA_HBASE_DIR}/hbase-common/target/generated-sources/native/")
 ############
-## Validate that we have C++ 14 support
+## Validate that we have C++ 17 support

Review comment:
       Is this for std::optional?

##########
File path: CMakeLists.txt
##########
@@ -154,6 +160,9 @@ endif (OPENSSL_FOUND)
 
 
 if (DOWNLOAD_DEPENDENCIES)
+  download_doubleconversion(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR})
+  download_fizz(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR})

Review comment:
       This needs corresponding changes in Dockerfile?

##########
File path: cmake/DownloadCyrusSasl.cmake
##########
@@ -0,0 +1,37 @@
+# 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.
+
+## Download Cyrus SASL
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+
+function(download_cyrus_sasl SOURCE_DIR BUILD_DIR)
+  ExternalProject_Add(
+      cyrussasl
+      URL "https://github.com/cyrusimap/cyrus-sasl/releases/download/cyrus-sasl-2.1.27/cyrus-sasl-2.1.27.tar.gz"
+      PREFIX "${BUILD_DIR}/dependencies"
+      SOURCE_DIR "${BUILD_DIR}/dependencies/cyrussasl-src"
+      BINARY_DIR ${BUILD_DIR}/dependencies/cyrussasl-src/
+      CONFIGURE_COMMAND ./configure --enable-static --with-pic --prefix=${BUILD_DIR}/dependencies/cyrussasl-install
+        "CFLAGS=-fPIC"
+  			"CXXFLAGS=${CMAKE_CXX_FLAGS} -fPIC"
+  )
+  add_library(sasl2 STATIC IMPORTED)

Review comment:
       Move this to CMakeLists like other dependencies?

##########
File path: cmake/DownloadWangle.cmake
##########
@@ -28,16 +29,21 @@ function(download_wangle SOURCE_DIR BUILD_DIR)
   else()
     set(PATCH_FOLLY "")
   endif() 
-	
+
   ExternalProject_Add(
-     facebook-wangle-proj
-     URL "https://github.com/facebook/wangle/archive/v2017.09.04.00.tar.gz"
-     PREFIX "${BUILD_DIR}/dependencies"
-     DOWNLOAD_DIR ${WANGLE_DOWNLOAD_DIR}
-     SOURCE_DIR ${WANGLE_SOURCE_DIR}
-     PATCH_COMMAND ${PATCH_FOLLY}
-     INSTALL_DIR ${WANGLE_INSTALL_DIR}
-     CONFIGURE_COMMAND ${CMAKE_COMMAND} -DBUILD_EXAMPLES=OFF -DCMAKE_CROSSCOMPILING=ON -DBUILD_TESTS=OFF -DFOLLY_ROOT_DIR=${FOLLY_ROOT_DIR} -DCMAKE_INSTALL_PREFIX:PATH=${WANGLE_INSTALL_DIR} "${WANGLE_SOURCE_DIR}/wangle" # Tell CMake to use subdirectory as source.
+	  facebook-wangle-proj
+	GIT_REPOSITORY "https://github.com/facebook/wangle.git"
+	GIT_TAG "v2020.05.18.00"
+	SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-wangle-proj-src"

Review comment:
       Same comment as above, you've undone the PREFIX path, so just want to be sure that the build directory is clean.

##########
File path: cmake/DownloadDoubleConversion.cmake
##########
@@ -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.
+
+## Download facebook's folly library. 
+## SOURCE_DIR is typically the cmake source directory
+function(download_doubleconversion SOURCE_DIR BUILD_DIR)

Review comment:
       nit: 2-4 indentation (sorry for nit-picking :-))

##########
File path: cmake/patches/zookeeper.3.4.14.buf
##########
@@ -0,0 +1,4 @@
+3480c3480
+<     static char buf[128];

Review comment:
       unused?

##########
File path: cmake/ProtobufGen.cmake
##########
@@ -36,6 +36,7 @@ function(generate_protobuf_src SRCS HDRS HDR_DIR PROTO_PATH)
   set(${SRCS})
   set(${HDRS})
   foreach(FIL ${ARGN})
+  	message("Generating ${FIL} with ${PROTOBUF_PROTOC_EXECUTABLE}")

Review comment:
       indentation off

##########
File path: cmake/DownloadDoubleConversion.cmake
##########
@@ -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.
+
+## Download facebook's folly library. 

Review comment:
       Download "double conv"... ? Looks like a copy-paste mistake.

##########
File path: cmake/DownloadCyrusSasl.cmake
##########
@@ -0,0 +1,37 @@
+# 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.
+
+## Download Cyrus SASL
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+
+function(download_cyrus_sasl SOURCE_DIR BUILD_DIR)
+  ExternalProject_Add(
+      cyrussasl
+      URL "https://github.com/cyrusimap/cyrus-sasl/releases/download/cyrus-sasl-2.1.27/cyrus-sasl-2.1.27.tar.gz"
+      PREFIX "${BUILD_DIR}/dependencies"
+      SOURCE_DIR "${BUILD_DIR}/dependencies/cyrussasl-src"
+      BINARY_DIR ${BUILD_DIR}/dependencies/cyrussasl-src/
+      CONFIGURE_COMMAND ./configure --enable-static --with-pic --prefix=${BUILD_DIR}/dependencies/cyrussasl-install
+        "CFLAGS=-fPIC"
+  			"CXXFLAGS=${CMAKE_CXX_FLAGS} -fPIC"

Review comment:
       nit: unnecessary line break

##########
File path: cmake/DownloadDoubleConversion.cmake
##########
@@ -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.
+
+## Download facebook's folly library. 
+## SOURCE_DIR is typically the cmake source directory
+function(download_doubleconversion SOURCE_DIR BUILD_DIR)
+	ExternalProject_Add(
+		doubleconversion-proj
+		GIT_REPOSITORY "https://github.com/google/double-conversion.git"
+		GIT_TAG "master"
+		SOURCE_DIR "${BUILD_DIR}/dependencies/doubleconversion-proj-src"
+		CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}

Review comment:
       Whats with the PASSTHROUGH_CMAKE_ARGS? Seems not set anywhere, can users override?

##########
File path: cmake/DownloadDoubleConversion.cmake
##########
@@ -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.
+
+## Download facebook's folly library. 
+## SOURCE_DIR is typically the cmake source directory
+function(download_doubleconversion SOURCE_DIR BUILD_DIR)
+	ExternalProject_Add(
+		doubleconversion-proj
+		GIT_REPOSITORY "https://github.com/google/double-conversion.git"
+		GIT_TAG "master"

Review comment:
       Just to be sure, we are not setting prefix dir here, it doesn't download stuff into build root right? I haven't tried the patch yet, just want to be sure that it doesn't mess the build directory after my last cleanup.

##########
File path: cmake/DownloadFolly.cmake
##########
@@ -17,21 +17,21 @@
 
 ## Download facebook's folly library. 
 ## SOURCE_DIR is typically the cmake source directory
-## BUILD_DIR is the build directory, typically 'build'
-
 function(download_folly SOURCE_DIR BUILD_DIR)
   ExternalProject_Add(
-      facebook-folly-proj
-      # TODO: Source version information from cmake file.
-      URL "https://github.com/facebook/folly/archive/v2017.09.04.00.tar.gz"
-      PREFIX "${BUILD_DIR}/dependencies"
-      SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-folly-proj-src"
-      BINARY_DIR ${BUILD_DIR}/dependencies/facebook-folly-proj-src/folly
-      CONFIGURE_COMMAND autoreconf -ivf
-      COMMAND ./configure  --prefix=${BUILD_DIR}/dependencies/facebook-folly-proj-install
-          "CFLAGS=-fPIC -lboost_context -lboost_coroutine -ldl" ## this version of folly does not support cmake so we must pass args manually
-  	   "CXXFLAGS=${CMAKE_CXX_FLAGS} -fPIC -lboost_context -lboost_coroutine -ldl" ## this version of folly does not support cmake so we must pass args manually
-      UPDATE_COMMAND ""
+    facebook-folly-proj
+    GIT_REPOSITORY "https://github.com/facebook/folly.git"
+    GIT_TAG "v2020.05.18.00"
+    SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-folly-proj-src"
+    PATCH_COMMAND ${CMAKE_COMMAND} -E copy
+      		"${CMAKE_CURRENT_SOURCE_DIR}/cmake/doubleconversion/local/FindDoubleConversion.cmake" ${BUILD_DIR}/dependencies/facebook-folly-proj-src/CMake

Review comment:
       nit: indentation

##########
File path: cmake/DownloadFizz.cmake
##########
@@ -0,0 +1,41 @@
+# 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.
+
+## Download facebook's fizz library. 
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+function(download_fizz SOURCE_DIR BUILD_DIR)
+
+	
+	ExternalProject_Add(
+		facebook-fizz-proj
+		GIT_REPOSITORY "https://github.com/facebookincubator/fizz.git"
+		GIT_TAG "v2020.05.18.00"
+		SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-fizz-proj-src"
+		PATCH_COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/folly/local/FindFolly.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake/
+			COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/doubleconversion/local/FindDoubleConversion.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake

Review comment:
       nit: indentation.

##########
File path: cmake/doubleconversion/system/FindDoubleConversion.cmake
##########
@@ -14,7 +14,7 @@
 
 # Finds libdouble-conversion.
 #
-# DEFINE DOUBLE_CONVERSION_ROOT_DIR to provide a hint
+ DEFINE DOUBLE_CONVERSION_ROOT_DIR to provide a hint

Review comment:
       intentional?

##########
File path: src/hbase/client/async-batch-rpc-retrying-caller.cc
##########
@@ -70,7 +70,8 @@ AsyncBatchRpcRetryingCaller<REQ, RESP>::~AsyncBatchRpcRetryingCaller() {}
 template <typename REQ, typename RESP>
 Future<std::vector<Try<RESP>>> AsyncBatchRpcRetryingCaller<REQ, RESP>::Call() {
   GroupAndSend(actions_, 1);
-  return collectAll(action2futures_);
+  // use the executor to convert he SemiFuture to a Future.
+  return std::move(collectAll(action2futures_)).via(cpu_pool_.get());

Review comment:
       whats the need for move() everywhere? Isn't a copy elided with RVO?

##########
File path: cmake/patches/zookeeper.3.4.14.cli
##########
@@ -0,0 +1,4 @@
+559c559
+<         strncpy(cmd, argv[2]+4, sizeof(cmd));

Review comment:
       unused?

##########
File path: include/hbase/client/configuration.h
##########
@@ -28,8 +28,6 @@
 
 namespace hbase {
 
-template <class T>

Review comment:
       nice..

##########
File path: cmake/DownloadFizz.cmake
##########
@@ -0,0 +1,41 @@
+# 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.
+
+## Download facebook's fizz library. 
+## SOURCE_DIR is typically the cmake source directory
+## BINARY_DIR is the build directory, typically 'build'
+
+function(download_fizz SOURCE_DIR BUILD_DIR)
+
+	
+	ExternalProject_Add(
+		facebook-fizz-proj
+		GIT_REPOSITORY "https://github.com/facebookincubator/fizz.git"
+		GIT_TAG "v2020.05.18.00"
+		SOURCE_DIR "${BUILD_DIR}/dependencies/facebook-fizz-proj-src"
+		PATCH_COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/folly/local/FindFolly.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake/
+			COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_SOURCE_DIR}/cmake/doubleconversion/local/FindDoubleConversion.cmake" ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/cmake
+			COMMAND patch ${BUILD_DIR}/dependencies/facebook-fizz-proj-src/fizz/CMakeLists.txt "${CMAKE_CURRENT_SOURCE_DIR}/cmake/patches/fizz.v2020.05.18.00.cmake" 

Review comment:
       A comment about fizz.v2020.05.18.00.cmake would help 

##########
File path: src/test/hbase-configuration-test.cc
##########
@@ -241,7 +241,7 @@ TEST(Configuration, EnvVars) {
 
   HBaseConfigurationLoader loader;
   hbase::optional<Configuration> conf = loader.LoadDefaultResources();
-  ASSERT_TRUE(conf != none) << "No configuration object present.";
+  ASSERT_TRUE(conf ) << "No configuration object present.";

Review comment:
       trailing space every where, missed it in a sed command? :-D




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org