Matthew Jacobs has posted comments on this change. Change subject: IMPALA-1822: Cleanup query profile log ......................................................................
Patch Set 3: (6 comments) Just a few small things. Thanks! The be test looks nice. http://gerrit.cloudera.org:8080/#/c/2260/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 395: " why break this line? http://gerrit.cloudera.org:8080/#/c/2260/3/be/src/util/logging-support.cc File be/src/util/logging-support.cc: Line 100: typedef nit: space between this and below Line 102: if (max_log_files <= 0) return; nit: reorder this, makes sense to read this line first and have the typedef right next to the usage below Line 106: glob We should probably check the return value of this. If it's not zero, we should probably log an error and return (probably need to call globfree() to be safe). http://gerrit.cloudera.org:8080/#/c/2260/3/be/src/util/logging-support.h File be/src/util/logging-support.h: Line 36: string in header files we typically use namespaces, so std::string here (no need to change in the .cc file) http://gerrit.cloudera.org:8080/#/c/2260/3/be/src/util/simple-logger.cc File be/src/util/simple-logger.cc: Line 117: log_file_name A bit more concise (and probably more efficient) to use: Substitute("$0/$1*", log_dir_, log_file_name_prefix_); may need to import: #include <gutil/strings/substitute.h> -- To view, visit http://gerrit.cloudera.org:8080/2260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I52430d192d27cdf9bc0d81f26a63048eaeec5d29 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
