Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/9769 )
Change subject: IMPALA-6904: stress test threshold parameters ...................................................................... Patch Set 2: (6 comments) Everything functional looks fine. There are changes that introduce PEP-008 violations as explained by flake8. I get the tool flake8 isn't widely known. I'll try to spend some time on that. http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1242 PS2, Line 1242: def load_random_queries_and_populate_runtime_info( : query_generator, model_translator, tables, impala, converted_args): flake8 considers this a whitespace violation. The proper forms are: def function1(param1, param2, paramN): # all params on first line pass def function2( param1, param2, paramN ): # no params on first line, subsequent lines aligned together, # closing parenthesis aligned with "def" on its own line pass def function3(param1, param2, paramN): # params on multiple lines, but subsequent lines aligned with first param pass I believe the form from function2 was used here due to the long method name and long parameter list. http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1271 PS2, Line 1271: populate_runtime_info(query, impala, converted_args, : timeout_secs=converted_args.random_query_timeout_seconds) This is also a stylistic white space violation that was previously correct. See my comment at L1355 for more details. http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1294 PS2, Line 1294: 'samples' and 'max_conflicting_samples' control the reliability of the collected : information. The problem is that memory spilling or usage may differ (by a large : amount) from run to run due to races during execution. The parameters provide a way : to express "X out of Y runs must have resulted in the same outcome". Increasing the : number of samples and decreasing the tolerance (max conflicts) increases confidence : but also increases the time to collect the data. I think it's informative to keep this language here. Perhaps prepend the first sentence with "From converted_args, " ? http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1354 PS2, Line 1354: report.write_query_profile(os.path.join(results_dir, PROFILES_DIR), : profile_error_prefix) flake8 considers this a whitespace violation. The previous alignment was 1 of 2 ways to do it correctly. Both ways align all params together. The other is this style: report.write_query_profile(os.path.join(results_dir, PROFILES_DIR), profile_error_prefix) http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1359 PS2, Line 1359: report.write_query_profile(os.path.join(results_dir, PROFILES_DIR), : profile_error_prefix) This too will fail with flake8. http://gerrit.cloudera.org:8080/#/c/9769/2/tests/stress/concurrent_select.py@1369 PS2, Line 1369: report.write_query_profile(os.path.join(results_dir, PROFILES_DIR), : profile_error_prefix) ...and here. -- To view, visit http://gerrit.cloudera.org:8080/9769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I46cc95cbb078c5ef9886971ab1c0f493ddcf8377 Gerrit-Change-Number: 9769 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Nithya Janarthanan <njanartha...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Apr 2018 20:32:46 +0000 Gerrit-HasComments: Yes
