Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12152 )
Change subject: IMPALA-6521: Show experimental flags in /varz and log files ...................................................................... Patch Set 2: (3 comments) Generally seems good, had some style comments http://gerrit.cloudera.org:8080/#/c/12152/2/be/src/common/logging.cc File be/src/common/logging.cc: http://gerrit.cloudera.org:8080/#/c/12152/2/be/src/common/logging.cc@165 PS2, Line 165: string better to use a stringstream for building strings (appending to string can be quadratic, I think). http://gerrit.cloudera.org:8080/#/c/12152/2/be/src/common/logging.cc@166 PS2, Line 166: for (auto i = flags.begin(); i != iter; ++i) { I'm confused by this iteration logic. Why not something like: for (const auto& flag : flags) { if (!flag.hidden) continue; ... } http://gerrit.cloudera.org:8080/#/c/12152/2/tests/custom_cluster/test_web_pages.py File tests/custom_cluster/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/12152/2/tests/custom_cluster/test_web_pages.py@35 PS2, Line 35: filter I think list comprehensions are generally preferred to filter() in python - https://www.artima.com/weblogs/viewpost.jsp?thread=98196 -- To view, visit http://gerrit.cloudera.org:8080/12152 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic13b2694b445059e7e52c1ae56bd7e0e925fbb0e Gerrit-Change-Number: 12152 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 03 Jan 2019 23:23:31 +0000 Gerrit-HasComments: Yes
