Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18798 )
Change subject: IMPALA-6684: Fix untracked memory in KRPC ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/18798/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18798/2//COMMIT_MSG@20 PS2, Line 20: could you add a testing section to list what are tests have been done for this patch? http://gerrit.cloudera.org:8080/#/c/18798/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18798/4//COMMIT_MSG@20 PS4, Line 20: Please add a Testing section which list the tests I did for this patch, like performance test, manual real cluster test, precommit core test, new unit test, etc. http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/krpc-data-stream-sender.h@273 PS4, Line 273: boost::scoped_ptr For new code, we prefer to use std::unique_ptr, instead of boost::scoped_ptr. Same comments for next two variables. http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/krpc-data-stream-sender.cc@753 PS4, Line 753: // on some channel reset outbound_rb_mem_tracker_, outbound_rb_mem_pool_ and outbound_rb_free_pool_ as nullptr to free the objects. http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/krpc-data-stream-sender.cc@1108 PS4, Line 1108: krpc_tuple_data_bytes_ = : ADD_SUMMARY_STATS_COUNTER(profile(), "TupleDataBytes", TUnit::BYTES); : krpc_compression_scratch_bytes_ = : ADD_SUMMARY_STATS_COUNTER(profile(), "CompressionScratchBytes", TUnit::BYTES); Should we move these two lines to function KrpcDataStreamSender::Prepare() as other counters? http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.h@513 PS4, Line 513: tuple_offsets Don't see this parameter in function Serialize() signature. It is used for SerializeThrift http://gerrit.cloudera.org:8080/#/c/18798/3/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/18798/3/be/src/runtime/row-batch.h@514 PS3, Line 514: Don't see this parameter http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.cc@300 PS4, Line 300: As part of the serialization process we deduplicate tuples to avoid serializing a : // Tuple multiple times for the RowBatch. By default we only detect duplicate tuples : // in adjacent rows only. If full deduplication is enabled, we will build a : // map to detect non-adjacent duplicates. Building this map comes with significant : // overhead, so is only worthwhile in the uncommon case of many non-adjacent duplicates. The comments is duplicated with the one in line 347. Could you make one shorter? http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.cc@322 PS4, Line 322: if (full_dedup) { : RETURN_IF_ERROR( : Serialize(full_dedup, &distinct_tuples, output_batch, &uncompressed_size, : &is_compressed, size, perm_free_pool, compression_scratch_stats_counter)); : } else { : RETURN_IF_ERROR(Serialize(full_dedup, nullptr, output_batch, &uncompressed_size, : &is_compressed, size, perm_free_pool, compression_scratch_stats_counter)); : } Simplify as: RETURN_IF_ERROR(Serialize(full_dedup, full_dedup ? &distinct_tuples : nullptr, output_batch, &uncompressed_size, &is_compressed, size, perm_free_pool, compression_scratch_stats_counter)); http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.cc@404 PS4, Line 404: expected to be removed once migration from Thrift RPC to KRPC is done. I think we already migrate from Thrift RPC to KRPC for data communication between backends. But code may not be fully cleaned. http://gerrit.cloudera.org:8080/#/c/18798/4/be/src/runtime/row-batch.cc@416 PS4, Line 416: F nit: low case -- To view, visit http://gerrit.cloudera.org:8080/18798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ba2b907ce4f275a7a1fb8cf75453c7003eb4b82 Gerrit-Change-Number: 18798 Gerrit-PatchSet: 4 Gerrit-Owner: Omid Shahidi <omid.shahidi.2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Omid Shahidi <omid.shahidi.2...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Fri, 05 Aug 2022 05:34:04 +0000 Gerrit-HasComments: Yes
