github-actions[bot] commented on code in PR #66820:
URL: https://github.com/apache/doris/pull/66820#discussion_r3794852028
##########
gensrc/thrift/Makefile:
##########
@@ -30,9 +30,13 @@ all: ${GEN_OBJECTS} ${OBJECTS}
.PHONY: all
THRIFT_CPP_ARGS = -I ${CURDIR} -I ${BUILD_DIR}/thrift/ --gen
cpp:moveable_types,no_skeleton -out ${BUILD_DIR}/gen_cpp --allow-64bit-consts
-strict
+CONTAINER_MEMORY_CHECK = ${CURDIR}/add_container_memory_check.py
${BUILD_DIR}/gen_cpp:
mkdir -p $@
# handwrite thrift
+${BUILD_DIR}/gen_cpp/parquet_types.cpp: ${CONTAINER_MEMORY_CHECK}
Review Comment:
[P1] Regenerate every reader that needs this hook
The checker is a prerequisite only of `parquet_types.cpp`, while the pattern
targets for `Types`, `PlanNodes`, `PaloInternalService`, `RuntimeProfile`, and
the other generated readers still depend only on their unchanged IDL. The
normal `build.sh` path calls `generated-source.sh noclean`, so after an
incremental checkout those existing outputs remain newer and Make never reruns
this recipe; their `deserialize_thrift_msg()` callers therefore use
`MemoryBudgetProtocol` with generated `read()` methods that contain no
`reserve_thrift_container_memory()` call, leaving the original eager
large-struct resize/OOM path reachable in plan and service requests. Please
make the checker a normal prerequisite of every instrumented `%_types.cpp`
target, and cover an incremental regeneration without `--clean`.
##########
be/src/util/thrift_util.cpp:
##########
@@ -56,6 +61,66 @@ class TProtocol;
#include <thread>
namespace doris {
+namespace {
+
+class ScopedThreadContextHandle {
+public:
+ ScopedThreadContextHandle() {
ThreadLocalHandle::create_thread_local_if_not_exits(); }
+ ~ScopedThreadContextHandle() {
ThreadLocalHandle::del_thread_local_if_count_is_zero(); }
+};
+
+class MemoryBudgetProtocol final : public
apache::thrift::protocol::TProtocolDecorator,
+ public ThriftContainerMemoryChecker {
+public:
+ explicit
MemoryBudgetProtocol(std::shared_ptr<apache::thrift::protocol::TProtocol>
protocol)
+ : TProtocolDecorator(std::move(protocol)) {
+ _memory_manager = thread_context()->thread_mem_tracker_mgr.get();
+ if (_memory_manager->limiter_mem_tracker()->label() == "Orphan") {
+ // Apache Thrift worker threads have no Doris task context. Attach
a process-accounted
+ // limiter so reservation checks never run against the forbidden
orphan tracker.
+ _fallback_tracker =
MemTrackerLimiter::create_shared(MemTrackerLimiter::Type::OTHER,
+
"ThriftDeserialize");
+ _memory_manager->attach_limiter_tracker(_fallback_tracker);
+ _switched_tracker = true;
+ }
+ _prior_reservation = _memory_manager->take_reserved_memory();
+ }
+
+ ~MemoryBudgetProtocol() override {
Review Comment:
[P1] Keep successful decoded storage charged for its lifetime
These generated fields are ordinary `std::vector`s, and the current
malloc/new paths do not call `ThreadMemTrackerMgr::consume()`. Their resizes
therefore never convert `_reserved_mem` into persistent consumption, yet this
destructor releases the full reservation while `deserialized_msg` still owns
the vectors. `load_native_page_indexes()` repeats this for every selected
column and retains all of the decoded `ColumnIndex`/`OffsetIndex` objects, so
each call can pass the task/workload-group checks independently while their
aggregate live storage exceeds those budgets. Please make successful decoded
storage allocator-tracked or carry an owned charge for its actual lifetime,
shrink only unused reservation, and add a test that retains two individually
admissible outputs whose sum exceeds the limit.
--
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]
