Tim Armstrong 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 5: (9 comments) I think we could be a bit more defensive. I commented on the uses where the memory lifetime wasn't obviously longer than the lifetime of the JSON doc. 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: rapidjson::StringRef(args.find("query_id")->second.c_str()), Maybe make a defensive copy here too? I think args probably outlives the use of the document but it would be easy to introduce abug. http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@285 PS5, Line 285: rapidjson::StringRef(args.find("query_id")->second.c_str()), Same here - make a copy of the string? http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@584 PS5, Line 584: value->AddMember("label", rapidjson::StringRef((*it)->label.c_str()), Same here - make a copy? http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/service/impala-http-handler.cc@689 PS5, Line 689: rapidjson::StringRef(label_map[sink.stream_sink.dest_node_id].c_str()), Make a copy here too? 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: document->AddMember(rapidjson::StringRef(key_.c_str()), metric_list, Make a copy? We don't generally destroy metrics but it's hard to reason about the memory lifetime. http://gerrit.cloudera.org:8080/#/c/11124/5/be/src/util/collection-metrics.h@232 PS5, Line 232: document->AddMember(rapidjson::StringRef(key_.c_str()), container, Make a copy? 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 copying string refs below is safe Maybe add a comment here: // 'doc' uses the output document's allocator we we can safely return references to string values in 'doc' 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: document->AddMember(rapidjson::StringRef(key_.c_str()), val, Make a copy? 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: container.AddMember("name", rapidjson::StringRef(name_.c_str()), Make a copy? -- 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: 5 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 13 Aug 2018 16:58:04 +0000 Gerrit-HasComments: Yes
