Riza Suminto 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) 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/ I think we can do something like this in impala-config.sh export IMPALA_THRIFT_POM_VERSION=0.11.0 export IMPALA_THRIFT_JAVA_VERSION=${IMPALA_THRIFT_POM_VERSION}-p5 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. Interestingly, that line in requirements.txt is already ahead to thrift-0.14.2. Maybe we should resolve this in the next patch along with upgrading IMPALA_THRIFT_PY_VERSION? 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 think it is here in case we override THRIFT_CPP_VERSION in CMakeLists.txt. I can't find $ENV{THRIFT_CPP_VERSION} anywhere, so I assume this will be empty within cmake context. 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 th You're right. We should be able to remove the same part too for THRIFT_PY_. -- 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:44:08 +0000 Gerrit-HasComments: Yes
