Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/24358 )
Change subject: IMPALA-12955: Slim down profile tool deps ...................................................................... Patch Set 7: Code-Review+1 (4 comments) Thank you for pursuing this, it will make a nice difference. I only have a few nits. The project currently doesn't organize dependencies using CMake's target_link_libraries(), but I think that's what we'll want to do in the future. http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/CMakeLists.txt File be/src/common/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/CMakeLists.txt@30 PS7, Line 30: add_library(StackTraceUtil OBJECT : stack-trace.cc : ) : add_dependencies(StackTraceUtil gen-deps) Nit: I would prefer to put the new stack-trace.* files and library in be/src/util. We can specify it as a dependency of the Util library and that will get it to the binaries that need it, i.e. target_link_libraries(Util PUBLIC StackTraceUtil) http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/CMakeLists.txt@39 PS7, Line 39: status.cc Nit: Rather than including status this way (and adding StackTraceUtil below), I would rather list StatusMinimal as a dependency of Common. i.e. target_link_libraries(Common PUBLIC StatusMinimal) http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/CMakeLists.txt@48 PS7, Line 48: # Omit status-serialization.cc: impala-profile-tool needs normal Status : # construction but not Status thrift/proto conversion helpers. : add_library(ProfileToolCommon : status.cc : $<TARGET_OBJECTS:StackTraceUtil> : ) : add_dependencies(ProfileToolCommon gen-deps) Nit: Let's call this StatusMinimal, as we're likely to use this in other binaries in the future. Rather than including the stack trace utility via $<TARGET_OBJECTS:StackTraceUtil>, I would rather list it as a dependency of this library, i.e. target_link_libraries(StatusMinimal PRIVATE StackTraceUtil) http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/status.cc File be/src/common/status.cc: http://gerrit.cloudera.org:8080/#/c/24358/7/be/src/common/status.cc@159 PS7, Line 159: Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1) { : return Status(true, code, arg0, arg1); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2) { : return Status(true, code, arg0, arg1, arg2); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3) { : return Status(true, code, arg0, arg1, arg2, arg3); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4, const ArgType& arg5) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4, arg5); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4, const ArgType& arg5, const ArgType& arg6) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4, arg5, arg6); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4, const ArgType& arg5, const ArgType& arg6, : const ArgType& arg7) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4, const ArgType& arg5, const ArgType& arg6, : const ArgType& arg7, const ArgType& arg8) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8); : } : : Status Status::Expected(TErrorCode::type code, const ArgType& arg0, : const ArgType& arg1, const ArgType& arg2, const ArgType& arg3, : const ArgType& arg4, const ArgType& arg5, const ArgType& arg6, : const ArgType& arg7, const ArgType& arg8, const ArgType& arg9) { : return Status(true, code, arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, : arg9); : } Nit: We use a 90 character line length and the existing code is fine. Please remove these whitespace changes. -- To view, visit http://gerrit.cloudera.org:8080/24358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id476411dcf6c46079e2e2addc5cdd552bc23f8a1 Gerrit-Change-Number: 24358 Gerrit-PatchSet: 7 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 05 Jun 2026 23:49:03 +0000 Gerrit-HasComments: Yes
