Omid Shahidi has posted comments on this change. ( http://gerrit.cloudera.org:8080/18798 )
Change subject: IMPALA-6684: Fix untracked memory in KRPC ...................................................................... Patch Set 3: (6 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_) { > Use auto& to avoid copying OutboundRowBatch. Should probably delete the cop Done for auto& Not sure why copy ctor should be deleted? Are we talking about the copy ctor for OutBoundRowBatch or copy ctor on line 718? Can you elaborate? thanks! 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) { > Missing {} Done http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@1093 PS2, Line 1093: outbound_rb_mem_pool_->FreeAll(); > Missing {} Done http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/krpc-data-stream-sender.cc@1105 PS2, Line 1105: Status KrpcDataStreamSender::SerializeBatch( > Probably better to not add these inside the serialize timer scope Done 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: /// if tuple_data_length_ > tuple_data_capacity_ new allocation will be necessary > Either add the struct or remove the comment. in your opinion, which one would be nicer? having a struct or leaving it as is? http://gerrit.cloudera.org:8080/#/c/18798/2/be/src/runtime/row-batch.h@584 PS2, Line 584: > Maybe less casting if you use uint8_t* here Majority of casting happens after and before serialization when need to change it because of allocation, do you mean to completely use uint8_t* instead of char* for tuple_data or just the parameter of this function? Not sure how it can reduce casting. -- 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: 3 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:19:27 +0000 Gerrit-HasComments: Yes
