You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2022/06/17 17:18:15 UTC
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18636
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
However, Thrift serialization/deserialization between minor version is
mostly compatible with each other. So it is possible to have different
thrift compiler version for different target code. It is also beneficial
to do so because different component can then be upgraded independently.
This patch implement the infrastructure change required to do so. It
replace most of 'THRIFT_*' environment variable anc cmake variable with
'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++, Java,
and Python code accordingly. All three still refer to the same thrift
version (thrift-0.11.0).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
14 files changed, 384 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/1
--
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: newchange
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 18:18:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18636
to look at the new patch set (#4).
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M java/pom.xml
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
15 files changed, 291 insertions(+), 134 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/4
--
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: newpatchset
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/2/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/2/bin/impala-config.sh@756
PS2, Line 756: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/2/bin/impala-config.sh@757
PS2, Line 757: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/2/bin/impala-config.sh@758
PS2, Line 758: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 17:46:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 6: Code-Review+2
This looks good to me
--
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 22:01:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18636
to look at the new patch set (#6).
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M java/pom.xml
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
15 files changed, 314 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/6
--
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: newpatchset
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 18:44:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10806/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 19:19:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10805/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 18:05:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/6/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/6/bin/impala-config.sh@762
PS6, Line 762: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/6/bin/impala-config.sh@763
PS6, Line 763: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/6/bin/impala-config.sh@764
PS6, Line 764: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:37:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18636
to look at the new patch set (#3).
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M java/pom.xml
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
15 files changed, 286 insertions(+), 134 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/3
--
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: newpatchset
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8254/ DRY_RUN=false
--
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: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 22:04:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10815/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:57:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18636
to look at the new patch set (#2).
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
14 files changed, 384 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/2
--
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: newpatchset
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/3/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/3/bin/impala-config.sh@757
PS3, Line 757: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/3/bin/impala-config.sh@758
PS3, Line 758: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/3/bin/impala-config.sh@759
PS3, Line 759: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 19:00:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+1
Thanks for the changes! lgtm, but it would be great if someone with more cmake experience would also look at it
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jun 2022 05:52:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 4:
(1 comment)
This is looking good to me. I had one small comment.
http://gerrit.cloudera.org:8080/#/c/18636/4/common/thrift/CMakeLists.txt
File common/thrift/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/18636/4/common/thrift/CMakeLists.txt@131
PS4, Line 131: set(THRIFT_INCLUDE_DIR_OPTION -I ${THRIFT_CPP_CONTRIB_DIR} -I $ENV{HIVE_METASTORE_THRIFT_DIR}
: -I ${HIVE_THRIFT_SOURCE_DIR})
This is used for all three languages (see CPP_ARGS, JAVA_FE_ARGS, JAVA_EXT_DS_ARGS, PYTHON_ARGS, etc). We'll need to split it out for the different languages, so let's go ahead and do that.
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:15:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/5/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/5/bin/impala-config.sh@762
PS5, Line 762: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/5/bin/impala-config.sh@763
PS5, Line 763: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/5/bin/impala-config.sh@764
PS5, Line 764: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:34:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10804/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 17:38:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10814/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:53:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 7: Code-Review+2
--
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: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 22:04:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Reviewed-on: http://gerrit.cloudera.org:8080/18636
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M java/pom.xml
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
15 files changed, 314 insertions(+), 139 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 7: Verified+1
--
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: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jun 2022 02:40:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/1/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/1/bin/impala-config.sh@756
PS1, Line 756: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/1/bin/impala-config.sh@757
PS1, Line 757: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/1/bin/impala-config.sh@758
PS1, Line 758: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 17:19:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18636/4/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/18636/4/bin/impala-config.sh@762
PS4, Line 762: export THRIFT_CPP_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_CPP_VERSION}"
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/4/bin/impala-config.sh@763
PS4, Line 763: export THRIFT_JAVA_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_JAVA_VERSION}"
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/18636/4/bin/impala-config.sh@764
PS4, Line 764: export THRIFT_PY_HOME="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/thrift-${IMPALA_THRIFT_PY_VERSION}"
line too long (92 > 90)
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 19:09:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18636
to look at the new patch set (#5).
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
IMPALA-11369: Separate thrift compiler for different component
Impala used to have one thrift compiler version to compile C++, Java,
and Python code.
Most Thrift serialization/deserialization between minor versions are
compatible with each other. So it is possible to have different thrift
compiler versions for different target codes. It is beneficial to do so
because it will allow Impala to upgrade separate components
independently.
This patch implements the infrastructure change required to do so. It
replace most of the 'THRIFT_*' environment variable and CMake variable
with 'THRFIT_CPP_*', 'THRFIT_JAVA_*', and 'THRFIT_PY_*' to compile C++,
Java, and Python code accordingly. All three still refer to the same
thrift version (thrift-0.11.0-p5).
Testing:
- Build Impala and pass core tests.
Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
---
M CMakeLists.txt
M README-build.md
M be/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/set-pythonpath.sh
D cmake_modules/FindThrift.cmake
A cmake_modules/FindThriftCpp.cmake
A cmake_modules/FindThriftJava.cmake
A cmake_modules/FindThriftPython.cmake
M common/thrift/CMakeLists.txt
M java/pom.xml
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
15 files changed, 312 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/18636/5
--
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: newpatchset
Gerrit-Change-Id: I56479dc69b79024d1a4d09211bbe88a61fa0c6a4
Gerrit-Change-Number: 18636
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-11369: Separate thrift compiler for different component
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18636 )
Change subject: IMPALA-11369: Separate thrift compiler for different component
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10807/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jun 2022 19:29:27 +0000
Gerrit-HasComments: No