impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kathy Sun (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3716: Add Memory Tab in query's Details page
Date Tue, 19 Jul 2016 00:00:32 GMT
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 <kathy.sun@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message