Kathy Sun has posted comments on this change. Change subject: IMPALA-3716: Add Memory Tab in query's Details page ......................................................................
Patch Set 1: (10 comments) I change most of them. About this c_str() thing, I saw other part of code, they all have it. I think it's better to leave it this way. http://gerrit.cloudera.org:8080/#/c/3664/1/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: PS1, Line 242: > remove extra space Done PS1, Line 244: .c_str() > I don't think the .c_str() is necessary. It looks like Value has a construc I do think c_str() is needed. I remember removing it will have compile error. Even if it's not...I think it's better to leave it this way, to be consistent with other handler method in this file... PS1, Line 251: = "" > Don't need this initializer. The default value of a C++ string is "". Done Line 253: // Search the in-flight queries > Change the comment so it explains more "why". E.g. "Only in-flight queries Done PS1, Line 258: const string& > The type should be string, since we need to store the string somewhere (& w these lines is also copied from others (same file: line 669). Does it supposed to be just "string err = ......" ? Line 259: Value json_error(err.c_str(), document->GetAllocator()); > I don't think the .c_str() is necessary. It looks like Value has a construc same as above c_str() problem Line 267: if (!is_inflight) { > We can get rid of "is_inflight" and simplify the control flow by just doing Done PS1, Line 268: consumption > I think we should rename this to mem_usage_text. I think we should be consi Done Line 271: Value mem_usage(consumption.c_str(), document->GetAllocator()); > I don't think the .c_str() is necessary. It looks like Value has a construc same as above PS1, Line 273: .c_str() > I'm not sure if the .c_str() is necessary here either. same as above -- To view, visit http://gerrit.cloudera.org:8080/3664 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86db096ab7a022d230018becdb60bcc3056847af Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Kathy Sun <[email protected]> Gerrit-Reviewer: Kathy Sun <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
