github-actions[bot] commented on code in PR #65990:
URL: https://github.com/apache/doris/pull/65990#discussion_r3701115507
##########
fe/fe-thrift/pom.xml:
##########
@@ -87,7 +87,7 @@ under the License.
<artifactId>thrift-maven-plugin</artifactId>
<version>0.10.0</version>
<configuration>
- <generator>java:fullcamel</generator>
+
<generator>java:fullcamel,generated_annotations=suppress</generator>
Review Comment:
Block: this option is accepted by the 0.16 compiler, but the service sources
that compiler emits are not compatible with `libthrift` 0.24. Exact-head FE UT
build 1013047 uses the build-environment compiler, resolves the new 0.24
runtime, and then fails `fe-thrift` with 100
`ProcessFunction`/`AsyncProcessFunction` generic-arity errors across the
generated services. Please update and require the 0.24 compiler in every FE
build environment and supported override atomically with the runtime bump
(ideally with a fail-fast version check); the current head cannot pass the
standard FE build.
##########
thirdparty/vars.sh:
##########
@@ -68,10 +68,10 @@ OPENSSL_SOURCE=openssl-OpenSSL_1_1_1s
OPENSSL_MD5SUM="7e79a7560dee77c0758baa33c61af4b4"
# thrift
-THRIFT_DOWNLOAD="http://archive.apache.org/dist/thrift/0.16.0/thrift-0.16.0.tar.gz";
-THRIFT_NAME=thrift-0.16.0.tar.gz
-THRIFT_SOURCE=thrift-0.16.0
-THRIFT_MD5SUM="44cf1b54b4ec1890576c85804acfa637"
+THRIFT_DOWNLOAD="http://archive.apache.org/dist/thrift/0.24.0/thrift-0.24.0.tar.gz";
+THRIFT_NAME=thrift-0.24.0.tar.gz
+THRIFT_SOURCE=thrift-0.24.0
Review Comment:
This compiler bump also drives `regression-test/framework`, but that
standalone POM still pins `libthrift:0.16.0` and generates with
`java:fullcamel`. A 0.24 compiler emits recursion-depth calls missing from 0.16
`TProtocol` plus the new service base-class signatures, so the framework will
no longer compile after a normal third-party rebuild. Please align its
compiler/runtime and generator options (or pin a compatible compiler
explicitly) and include the framework compile in upgrade coverage.
##########
thirdparty/build-thirdparty.sh:
##########
@@ -412,9 +412,9 @@ build_thrift() {
# NOTE(amos): libtool discard -static. --static works.
./configure CFLAGS="${cflags}" CXXFLAGS="${cxxflags}" LDFLAGS="${ldflags}"
LIBS="-lcrypto -ldl -lssl" \
--prefix="${TP_INSTALL_DIR}" --docdir="${TP_INSTALL_DIR}/doc"
--enable-static --disable-shared --disable-tests \
- --disable-tutorial --without-qt4 --without-qt5 --without-csharp
--without-erlang --without-nodejs --without-nodets --without-swift \
- --without-lua --without-perl --without-php --without-php_extension
--without-dart --without-ruby --without-cl \
- --without-haskell --without-go --without-haxe --without-d
--without-python -without-java --without-dotnetcore -without-rs --with-cpp \
+ --disable-tutorial --without-c_glib --without-java --without-kotlin
--without-erlang --without-nodejs --without-nodets \
+ --without-lua --without-python --without-py3 --without-perl
--without-php --without-php_extension \
+ --without-dart --without-ruby --without-go --without-rs --without-cl
--without-netstd --without-d --with-cpp \
Review Comment:
Thrift 0.24 still supports and defaults the Qt5 library probe to enabled.
Dropping the previous `--without-qt5` means a host with Qt5 and `moc` now
builds/installs an unused `libthriftqt5`, while current clean CI only covers
hosts without Qt. Please restore `--without-qt5` so this C++-only third-party
build produces the same artifacts regardless of the developer/build image.
##########
thirdparty/build-thirdparty.sh:
##########
@@ -412,9 +412,9 @@ build_thrift() {
# NOTE(amos): libtool discard -static. --static works.
./configure CFLAGS="${cflags}" CXXFLAGS="${cxxflags}" LDFLAGS="${ldflags}"
LIBS="-lcrypto -ldl -lssl" \
--prefix="${TP_INSTALL_DIR}" --docdir="${TP_INSTALL_DIR}/doc"
--enable-static --disable-shared --disable-tests \
- --disable-tutorial --without-qt4 --without-qt5 --without-csharp
--without-erlang --without-nodejs --without-nodets --without-swift \
- --without-lua --without-perl --without-php --without-php_extension
--without-dart --without-ruby --without-cl \
- --without-haskell --without-go --without-haxe --without-d
--without-python -without-java --without-dotnetcore -without-rs --with-cpp \
+ --disable-tutorial --without-c_glib --without-java --without-kotlin
--without-erlang --without-nodejs --without-nodets \
Review Comment:
The documented in-place `build-thirdparty.sh thrift` upgrade is not safe.
This build puts `installed/include` before the in-tree headers and never
removes the 0.16 tree, so it fails as the PR notes. Even after manually
deleting those headers, replacing only Thrift leaves the installed
Arrow/Parquet archives compiled against the 0.16 C++ class/vtable definitions
while 0.24 changes `TProtocol`/`TTransport`. Please make this a fresh/staged
replacement or automatically clean Thrift-owned files and rebuild its C++
reverse dependencies, then cover a 0.16-to-0.24 incremental upgrade.
##########
fe/pom.xml:
##########
@@ -293,7 +293,7 @@ under the License.
<open-json.version>1.8</open-json.version>
<junit.version>5.14.1</junit.version>
<hikaricp.version>6.0.0</hikaricp.version>
- <thrift.version>0.16.0</thrift.version>
+ <thrift.version>0.24.0</thrift.version>
Review Comment:
The 0.24 runtime changes message-limit failures from `END_OF_FILE` /
`MaxMessageSize reached` to `MESSAGE_SIZE_LIMIT` / `Message size exceeds limit:
...`. Both `ResultReceiver` and `PointQueryExecutor` only text-match the old
message, so the documented `max_msg_size_of_result_receiver` guidance is lost
and the existing regression will fail; forwarded failures also render `cause:
null` because the new type is absent from `TYPE_MSG_MAP`. Please handle
`MESSAGE_SIZE_LIMIT` by type in both result paths, preserve the actionable
message, and add it to the forwarding map.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
