Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21476 )
Change subject: IMPALA-13130: Prioritize EndDataStream messages ...................................................................... Patch Set 2: (4 comments) Can you craft a BE test for this patch under data-stream-test.cc? Maybe a combination of DataStreamTestShortServiceQueue and DataStreamTest_Cancel to show that EndDataStream RPC can move ahead in the queue? http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/rpc/impala-service-pool.cc@187 PS2, Line 187: service_mem_tracker_->Consume(transfer_size); Looks like we intentionally ignore HARD MemLimit here. Please clarify in comment. It might be good to log WARNING if such thing happen. if (UNLIKELY(service_mem_tracker_->AnyLimitExceeded(MemLimit::HARD))) { LOG(WARNING) << "...."; } http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/runtime/krpc-data-stream-sender.cc@65 PS2, Line 65: 1 day Maybe something shorter like 1 hour is better? I understand that the queue size itself is only limited by memory. So short timeout ensure they will be prioritized over others. Status DataStreamService::Init() { int num_svc_threads = FLAGS_datastream_service_num_svc_threads > 0 ? FLAGS_datastream_service_num_svc_threads : CpuInfo::num_cores(); // The maximum queue length is set to maximum 32-bit value. Its actual capacity is // bound by memory consumption against 'mem_tracker_'. RETURN_IF_ERROR(ExecEnv::GetInstance()->rpc_mgr()->RegisterService(num_svc_threads, std::numeric_limits<int32_t>::max(), this, mem_tracker(), ExecEnv::GetInstance()->rpc_metrics())); return Status::OK(); } http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/runtime/krpc-data-stream-sender.cc@65 PS2, Line 65: other RPCs nit: "other DataStreamService RPCs" http://gerrit.cloudera.org:8080/#/c/21476/2/be/src/runtime/krpc-data-stream-sender.cc@685 PS2, Line 685: rpc_controller_.set_timeout( : MonoDelta::FromMilliseconds(FLAGS_data_stream_sender_eos_timeout_ms)); nit: Are we leaving other RPC of DataStreamService with no timeout set? Should we explicitly set them as MonoTime::Max()? -- To view, visit http://gerrit.cloudera.org:8080/21476 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2829e1ab5bcde36107e10bff5fe629c5ee60f3e8 Gerrit-Change-Number: 21476 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 04 Jun 2024 18:57:29 +0000 Gerrit-HasComments: Yes
