Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18602 )
Change subject: IMPALA-9615: re2's max_mem opt configurable via an Impala startup flag ...................................................................... Patch Set 7: (21 comments) http://gerrit.cloudera.org:8080/#/c/18602/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18602/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-9615: re2's max_mem opt configurable via an Impala startup flag : > Please shorten the commit title to fit within one line. Done http://gerrit.cloudera.org:8080/#/c/18602/2//COMMIT_MSG@21 PS2, Line 21: _limi > nit: amount Done http://gerrit.cloudera.org:8080/#/c/18602/2//COMMIT_MSG@22 PS2, Line 22: th no > nit: greater Done http://gerrit.cloudera.org:8080/#/c/18602/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18602/6//COMMIT_MSG@15 PS6, Line 15: The re2_mem_limit flag will : accept a memory specific > The re2_mem_limit will accept a memory specification string Done http://gerrit.cloudera.org:8080/#/c/18602/6//COMMIT_MSG@21 PS6, Line 21: re2_mem_limit fla > re2_mem_limit Done http://gerrit.cloudera.org:8080/#/c/18602/7/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/18602/7/be/src/common/global-flags.cc@230 PS7, Line 230: : DEFINE_string(re2_mem_limit, "8MB", : "Maximum bytes of memory to be used by re2's regex engine " : "to hold the compiled form of the regexp. For more memory-consuming patterns, " : "this can be set to be a higher number. Default to 8MB."); Sorry that I miss this before, but can you insert MEM_UNITS_HELP_MSG from constant-strings.h into this flag documentation please. http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/common/init.cc File be/src/common/init.cc: http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/common/init.cc@346 PS6, Line 346: re2_mem_limit > nit: re2_mem_limit Done http://gerrit.cloudera.org:8080/#/c/18602/2/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18602/2/be/src/exprs/string-functions-ir.cc@46 PS2, Line 46: (re2_mem_limit); > Thank you for the suggestion! I agree it will be nicer, I will make these c Done http://gerrit.cloudera.org:8080/#/c/18602/2/be/src/exprs/string-functions-ir.cc@940 PS2, Line 940: . By default, it u > nit: Impala code style does not prefix local variable with underscore. So t Done http://gerrit.cloudera.org:8080/#/c/18602/4/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18602/4/be/src/exprs/string-functions-ir.cc@941 PS4, Line 941: // resulting in slower execution : void StringFunctions::SetRE2MemOpt(re2::RE2::Options* opts) { > You can validate this flag once in impala::InitCommonRuntime (be/src/common Done http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/exprs/string-functions-ir.cc@53 PS6, Line 53: uint64_t StringFunctions::re2_mem_limit_ = 8 << 20 > having an initialized static member variable in header file of c++ is not p Thanks for the explanation. Keeping it like this should be sufficient then. http://gerrit.cloudera.org:8080/#/c/18602/7/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18602/7/be/src/exprs/string-functions-ir.cc@943 PS7, Line 943: // bool is_percent = false; // not used : // int64_t re2_max_mem_usage = : // ParseUtil::ParseMemSpec(FLAGS_re2_mem_limit, &is_percent, 0); : // // FIXME: The CLEAN_EXIT_WITH_ERROR throws an error, I am not sure why : // if (re2_max_mem_usage <= 0) { : // CLEAN_EXIT_WITH_ERROR( : // Substitute("Invalid mem limit for re2's regex engine: $0", : // FLAGS_re2_mem_limit)); : // } This comment can be removed? http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/exprs/string-functions.h@159 PS6, Line 159: re2_mem_limit) > nit: re2_mem_limit Done http://gerrit.cloudera.org:8080/#/c/18602/6/be/src/exprs/string-functions.h@207 PS6, Line 207: re2_mem_limit > nit: re2_mem_limit_ Done http://gerrit.cloudera.org:8080/#/c/18602/7/temp.log File temp.log: http://gerrit.cloudera.org:8080/#/c/18602/7/temp.log@1 PS7, Line 1: rootLoggerLevel = INFO Is this file accidentally committed? http://gerrit.cloudera.org:8080/#/c/18602/2/tests/custom_cluster/test_re2_max_mem.py File tests/custom_cluster/test_re2_max_mem.py: http://gerrit.cloudera.org:8080/#/c/18602/2/tests/custom_cluster/test_re2_max_mem.py@31 PS2, Line 31: > that is true and seems more readable to me. But also there's the case that Done http://gerrit.cloudera.org:8080/#/c/18602/2/tests/custom_cluster/test_re2_max_mem.py@32 PS2, Line 32: > nit: in Done http://gerrit.cloudera.org:8080/#/c/18602/2/tests/custom_cluster/test_re2_max_mem.py@42 PS2, Line 42: > nit: will Done http://gerrit.cloudera.org:8080/#/c/18602/2/tests/custom_cluster/test_re2_max_mem.py@44 PS2, Line 44: else: > nit: use permalink to refer to github Done http://gerrit.cloudera.org:8080/#/c/18602/6/tests/custom_cluster/test_re2_max_mem.py File tests/custom_cluster/test_re2_max_mem.py: http://gerrit.cloudera.org:8080/#/c/18602/6/tests/custom_cluster/test_re2_max_mem.py@22 PS6, Line 22: class TestRE2MaxMem(CustomClu > Unused variable, please remove. Done http://gerrit.cloudera.org:8080/#/c/18602/7/tests/custom_cluster/test_re2_max_mem.py File tests/custom_cluster/test_re2_max_mem.py: http://gerrit.cloudera.org:8080/#/c/18602/7/tests/custom_cluster/test_re2_max_mem.py@22 PS7, Line 22: class TestRE2MaxMem(CustomClusterTestSuite): > flake8: E302 expected 2 blank lines, found 1 Need 2 blank line between"from tests..." and "class TestRE2MaxMem..." -- To view, visit http://gerrit.cloudera.org:8080/18602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf28d2f7217b1322ab8fdfb2c02fff0608078571 Gerrit-Change-Number: 18602 Gerrit-PatchSet: 7 Gerrit-Owner: Omid Shahidi <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Omid Shahidi <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 16 Jun 2022 00:50:15 +0000 Gerrit-HasComments: Yes
