Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component ...................................................................... Patch Set 2: (4 comments) Looked through the solution quickly - I am very from a cmake expert, so I am unsure about those parts. What seems a bit incomplete to me is that while the env vars suggest that they control the version of Thrift completely, in Java it only controls the compiler, and in python I think it controls the library only partially. http://gerrit.cloudera.org:8080/#/c/18636/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18636/2//COMMIT_MSG@20 PS2, Line 20: THRFIT_JAVA_ Shouldn't we use this in https://github.com/apache/impala/blob/master/java/pom.xml#L50 ? http://gerrit.cloudera.org:8080/#/c/18636/2//COMMIT_MSG@20 PS2, Line 20: THRFIT_PY_ Shouldn't this be connected to impala-shell's thrift version somehow, e.g. https://github.com/apache/impala/blob/master/shell/packaging/requirements.txt#L9 ? http://gerrit.cloudera.org:8080/#/c/18636/2/cmake_modules/FindThriftCpp.cmake File cmake_modules/FindThriftCpp.cmake: http://gerrit.cloudera.org:8080/#/c/18636/2/cmake_modules/FindThriftCpp.cmake@24 PS2, Line 24: ant Wonder where this come from originally. I also don't get how is THRIFT_CPP_VERSION defined here - don't we get it from impala-config.sh? http://gerrit.cloudera.org:8080/#/c/18636/2/cmake_modules/FindThriftJava.cmake File cmake_modules/FindThriftJava.cmake: http://gerrit.cloudera.org:8080/#/c/18636/2/cmake_modules/FindThriftJava.cmake@66 PS2, Line 66: if (THRIFT_JAVA_LIB) : set(THRIFT_JAVA_LIBS ${THRIFT_JAVA_LIB}) : set(THRIFT_JAVA_STATIC_LIB ${THRIFT_JAVA_STATIC_LIB_PATH}/libthrift.a) : exec_program(${THRIFT_JAVA_COMPILER} : ARGS -version OUTPUT_VARIABLE THRIFT_JAVA_VERSION RETURN_VALUE THRIFT_JAVA_RETURN) : if (NOT THRIFT_JAVA_FIND_QUIETLY) : message(STATUS "Thrift version: ${THRIFT_JAVA_VERSION}") : endif () : set(THRIFT_JAVA_FOUND TRUE) : # for static linking with Thrift, THRIFT_JAVA_STATIC_LIB is set in FindThrift.cmake : add_library(thriftstatic_java STATIC IMPORTED) : set_target_properties(thriftstatic_java PROPERTIES IMPORTED_LOCATION ${THRIFT_JAVA_STATIC_LIB}) This shouldn't be needed in Java, right? I think that we should only get the compiler, as the lib will be pulled by maven based on the java Thrift version. -- To view, visit http://gerrit.cloudera.org:8080/18636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4 Gerrit-Change-Number: 18636 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 17 Jun 2022 18:18:29 +0000 Gerrit-HasComments: Yes
