[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has removed Jim Apple from this change.  ( 
http://gerrit.cloudera.org:8080/10100 )

Change subject: IMPALA-6923: Update scripts in benchmark folder to store 
workload and few minor updates
..


Removed reviewer Jim Apple.
--
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: deleteReviewer
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jinchul Kim 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has removed Jinchul Kim from this change.  ( 
http://gerrit.cloudera.org:8080/10100 )

Change subject: IMPALA-6923: Update scripts in benchmark folder to store 
workload and few minor updates
..


Removed reviewer Jinchul Kim.
--
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: deleteReviewer
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has removed Mostafa Mokhtar from this change.  ( 
http://gerrit.cloudera.org:8080/10100 )

Change subject: IMPALA-6923: Update scripts in benchmark folder to store 
workload and few minor updates
..


Removed reviewer Mostafa Mokhtar.
--
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: deleteReviewer
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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)

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 import
Done

Removed this import since it is not used in the file


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_su
Done. Good catch


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
Done


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-f
Done


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 o
Done



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Jinchul Kim 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Fri, 20 Jul 2018 18:11:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Michael Brown (Code Review)]

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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Jinchul Kim 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Wed, 18 Jul 2018 23:38:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#12). ( 
http://gerrit.cloudera.org:8080/10100 )

Change subject: IMPALA-6923: Update scripts in benchmark folder to store 
workload and few minor updates
..

IMPALA-6923: Update scripts in benchmark folder to store workload and few minor 
updates

Updates to the scripts in benchmark folder to
 - Store test execution results to ExecutionResults table
 - Store workload metrics to WorkloadMetrics
 - Option to mark a performance run as official
 - Option to skip saving of profiles

Testing: Executed these script to create the database and
populate the tables with the above mentioned data

Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
---
M tests/benchmark/create_database.py
M tests/benchmark/perf_result_datastore.py
M tests/benchmark/report_benchmark_results.py
3 files changed, 303 insertions(+), 76 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/10100/12
--
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: newpatchset
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 12
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Jinchul Kim 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10100/5/tests/benchmark/report_benchmark_results.py
File tests/benchmark/report_benchmark_results.py:

http://gerrit.cloudera.org:8080/#/c/10100/5/tests/benchmark/report_benchmark_results.py@735
PS5, Line 735: if not first_exec_summary:
> Add comment.
Done


http://gerrit.cloudera.org:8080/#/c/10100/5/tests/benchmark/report_benchmark_results.py@1058
PS5, Line 1058:   if exec_summaries[0] is None:
> Same as comment above, please add a comment line explaining what this does.
Done



--
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: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Mon, 14 May 2018 23:59:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6923: Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG@7
PS8, Line 7: Update
> Please add a space in front of this.
Done


http://gerrit.cloudera.org:8080/#/c/10100/5/tests/benchmark/perf_result_datastore.py
File tests/benchmark/perf_result_datastore.py:

http://gerrit.cloudera.org:8080/#/c/10100/5/tests/benchmark/perf_result_datastore.py@298
PS5, Line 298: LOG.debug('Inserting File Type: {0}, {1}, {2}'.format(
> Fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py
File tests/benchmark/report_benchmark_results.py:

http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@141
PS8, Line 141: Dont
> I think either "Don't" or "Do not" is better than Dont. This is used for us
Done


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@1058
PS8, Line 1058:   if exec_summaries[0] is None:
  : return ""
> I think this is a redundant special handling.  I believe L1054-1055 can be
Done



--
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: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Mon, 14 May 2018 23:52:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6923:Update scripts in benchmark folder to store workload and few minor updates

[Kim Jin Chul (Code Review)]

Kim Jin Chul 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 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG@7
PS8, Line 7: Update
Please add a space in front of this.


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py
File tests/benchmark/report_benchmark_results.py:

http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@141
PS8, Line 141: Dont
I think either "Don't" or "Do not" is better than Dont. This is used for user's 
helper message.


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@1058
PS8, Line 1058:   if exec_summaries[0] is None:
  : return ""
I think this is a redundant special handling.  I believe L1054-1055 can be 
merged into the L1058-1059 because there is no symbol dependent issue.

e.g.)
if options.hive_results or exec_summaries[0] is None:
  return ""



--
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: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Thu, 10 May 2018 00:12:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6923:Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/10100 )

Change subject: IMPALA-6923:Update scripts in benchmark folder to store 
workload and few minor updates
..

IMPALA-6923:Update scripts in benchmark folder to store workload and few minor 
updates

Updates to the scripts in benchmark folder to
 - Store test execution results to ExecutionResults table
 - Store workload metrics to WorkloadMetrics
 - Option to mark a performance run as official
 - Option to skip saving of profiles

Testing: Executed these script to create the database and
populate the tables with the above mentioned data

Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
---
M tests/benchmark/create_database.py
M tests/benchmark/perf_result_datastore.py
M tests/benchmark/report_benchmark_results.py
3 files changed, 252 insertions(+), 74 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/10100/8
--
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: newpatchset
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-6923:Update scripts in benchmark folder to store workload and few minor updates

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10100/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10100/5//COMMIT_MSG@7
PS5, Line 7: CDH-65183 Update scripts in benchmark folder to store workload and 
few minor updates
> Please use IMPALA tickets. People who do not work at Cloudera cannot read C
Done



--
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: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Tue, 24 Apr 2018 22:36:08 +
Gerrit-HasComments: Yes