Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/10100 )
Change subject: IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates ...................................................................... Patch Set 12: (5 comments) I think the changes are OK for the most part, but they could stand to use better Python conventions. One of my inline comments explains how to do that. http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py File tests/benchmark/perf_result_datastore.py: http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py@23 PS12, Line 23: from subprocess import * "*" is an antipattern for imports. It is better to be explicit about imported symbols. http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py@63 PS12, Line 63: def insert_workload_summary(self, run_info_id): : self._insert_workload_summary(run_info_id) This doesn't seem to do anything useful, so just rename _insert_workload_summary to insert_workload_summary and remove this. http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py@85 PS12, Line 85: dont_save_profiles): Negatives are weird. It would be better to call this save_profiles and flip conditions. The reason being, readability for stuff like: if not dont_save_profiles gets confusing. http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py@88 PS12, Line 88: temp_profile_folder = os.path.join(home_dir,"workspace","impala-workload-runner","profiles") This is badly formated. There should be spaces between parameters. impala-flake8 perf_result_datastore.py should help find problems like this. Please run it against this file. You don't have to fix all the existing flake8 errors. But for your own submissions, you should format them better. You can do that via "git diff HEAD^ | impala-flake8 --diff" from the root Impala directory. http://gerrit.cloudera.org:8080/#/c/10100/12/tests/benchmark/perf_result_datastore.py@155 PS12, Line 155: self._cache: Dictionary of with run_info/user as key/value This isn't quite right. self._cache appears to be a hash in which a tuple of (run_info, user) is the key. -- To view, visit http://gerrit.cloudera.org:8080/10100 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c Gerrit-Change-Number: 10100 Gerrit-PatchSet: 12 Gerrit-Owner: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Jinchul Kim <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Nithya Janarthanan <[email protected]> Gerrit-Comment-Date: Wed, 18 Jul 2018 23:38:37 +0000 Gerrit-HasComments: Yes
