Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18798 )
Change subject: IMPALA-6684: Fix untracked memory in KRPC ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@713 PS2, Line 713: for (auto batch : outbound_batches_) { > Done for auto& Deepends if we actually need to copy OutBoundRowBatch anywhere. Otherwise leaving the copy constructor can lead to these kinds of efficiency issues or worst if the copy constructor is not implemented correctly. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.h@124 PS2, Line 124: /// TODO: this can probably be a struct > in your opinion, which one would be nicer? having a struct or leaving it as I don't think the struct will simplify the code since these are not passed as arguments anywhere. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.cc@340 PS2, Line 340: Status RowBatch::Serialize(bool full_dedup, DedupMap* distinct_tuples, > Add comment explaining why SerializeThrift was added. Please reply on this. The code here appears to have some duplication so we should have an explanation why there is two versions. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.cc@471 PS2, Line 471: vector<int32_t>* tuple_offsets, char* tuple_data) { > Maybe less casting if you use uint8_t* here Did you try changing these and it made things worse? -- 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: 2 Gerrit-Owner: Omid Shahidi <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Omid Shahidi <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 04 Aug 2022 17:33:58 +0000 Gerrit-HasComments: Yes
