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: (13 comments) http://gerrit.cloudera.org:8080/#/c/18798/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18798/2//COMMIT_MSG@10 PS2, Line 10: which will hold the compressed tuple data for an outbound row batch. nit: double space "for an" http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.h@24 PS2, Line 24: #include <vector> switch with line 22 to reduce diffs. 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_) { Use auto& to avoid copying OutboundRowBatch. Should probably delete the copy ctor or make it private too. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@1092 PS2, Line 1092: if (outbound_rb_mem_pool_ != nullptr) outbound_rb_mem_pool_->FreeAll(); Missing {} http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@1093 PS2, Line 1093: if (outbound_rb_mem_tracker_ != nullptr) outbound_rb_mem_tracker_->Close(); Missing {} http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@1105 PS2, Line 1105: krpc_tuple_data_bytes_ = Probably better to not add these inside the serialize timer scope 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@93 PS2, Line 93: const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(tuple_data_)), shouldn't need const here since tuple_data_ not const http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.h@106 PS2, Line 106: pool->Free(const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(tuple_data_))); shouldn't need const here since tuple_data_ not const 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 Either add the struct or remove the comment. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.h@584 PS2, Line 584: vector<int32_t>* tuple_offsets, char* tuple_data); Maybe less casting if you use uint8_t* here 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. http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.cc@396 PS2, Line 396: Status RowBatch::Serialize_Thrift(bool full_dedup, vector<int32_t>* tuple_offsets, Rename to SerializeThrift 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 -- 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 <omid.shahidi.2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Aug 2022 22:03:18 +0000 Gerrit-HasComments: Yes
