Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11124 )
Change subject: IMPALA-7364: Bump RapidJSON version to 1.1.0 ...................................................................... Patch Set 6: (9 comments) Thank you for your patient review. I've made the strings safer now. http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@227 PS5, Line 227: document->AddMember("query_id", query_id, document->GetAllocator()); > Maybe make a defensive copy here too? I think args probably outlives the us Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@285 PS5, Line 285: } > Same here - make a copy of the string? Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@584 PS5, Line 584: // Node "details" may contain exprs which should be redacted. > Same here - make a copy? Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@689 PS5, Line 689: } > Make a copy here too? Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/collection-metrics.h File be/src/util/collection-metrics.h: http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/collection-metrics.h@97 PS5, Line 97: rapidjson::Value key(key_.c_str(), document->GetAllocator()); > Make a copy? We don't generally destroy metrics but it's hard to reason abo Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/collection-metrics.h@232 PS5, Line 232: rapidjson::Value key(key_.c_str(), document->GetAllocator()); > Make a copy? Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/default-path-handlers.cc@212 PS5, Line 212: Document doc(&document->GetAllocator()); > I guess we allocated 'doc' with the same allocator as 'document' so not cop Done. I think we should use MOVE semantic here. Since we're transferring members to the target document. Ref: http://rapidjson.org/md_doc_tutorial.html#MoveSemantics https://github.com/Tencent/rapidjson/blob/v1.1.0/include/rapidjson/document.h#L1181 https://github.com/Tencent/rapidjson/blob/v1.1.0/include/rapidjson/document.h#L1984 http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/metrics.h@169 PS5, Line 169: rapidjson::Value key(key_.c_str(), document->GetAllocator()); > Make a copy? Done http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/metrics.cc File be/src/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/metrics.cc@183 PS5, Line 183: Value name(name_.c_str(), document->GetAllocator()); > Make a copy? Done -- To view, visit http://gerrit.cloudera.org:8080/11124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21353b0d769f81c13f506737e41fbac17655245c Gerrit-Change-Number: 11124 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 14 Aug 2018 00:35:59 +0000 Gerrit-HasComments: Yes
