[Impala-ASF-CR] IMPALA-6160: Allow multiple statements in a Query object.

[Jim Apple (Code Review)]

Jim Apple has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8513 )

Change subject: IMPALA-6160: Allow multiple statements in a Query object.
..

IMPALA-6160: Allow multiple statements in a Query object.

Testing:
- Reproduced problem with bin/run-workload.py.
- Ran bin/run-workload.py --workloads=tpch,targeted-perf,tpcds
  --impalads=localhost:21000,localhost:21001,localhost:21002
  --results_json_file=$PWD/perf_results/IMPALA-6160.json
  --query_iterations=3 --table_formats=parquet/none --plan_first
  --query_names='.*' (Close to command line that single_node_perf_run.py
  builds.)
- Manually reviewed perf_results/IMPALA-6160.json to verify presence of
  plans and proper splitting of query batches.

Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Reviewed-on: http://gerrit.cloudera.org:8080/8513
Tested-by: Impala Public Jenkins
Reviewed-by: Jim Apple 
---
M tests/performance/query.py
M tests/performance/query_executor.py
2 files changed, 60 insertions(+), 25 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Jim Apple: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/8513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Gerrit-Change-Number: 8513
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Wood 

[Impala-ASF-CR] IMPALA-6160: Allow multiple statements in a Query object.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8513 )

Change subject: IMPALA-6160: Allow multiple statements in a Query object.
..


Patch Set 7: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Gerrit-Change-Number: 8513
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:38:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8544 )

Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
..


Patch Set 1:

(10 comments)

Testing (in a dry run; will not talk back to gerrit) here: 
https://jenkins.impala.io/job/gerrit-verify-dryrun/1474/

http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@9
PS1, Line 9:
nit: no space before comma


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@9
PS1, Line 9: To preserve case sensitivity in column labels , this patch 
modifies the getColumnLabel()
Please wrap commit messages at 72 characters. You can do this in emacs (if you 
use emacs for your commit message writing) by hitting ctrl-q.


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@10
PS1, Line 10: return original
"return the original"


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@10
PS1, Line 10: However since
nit: "However, since"


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@11
PS1, Line 11: lowercase a
nit: "lowercase, a"


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@12
PS1, Line 12: SelectStmt.java.lowercase
This is not a packagae name. Reword?


http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@12
PS1, Line 12: causing union test to fail hence replaced
nit: "causing the union test to fail, hence I replace"


http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java:

http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@97
PS1, Line 97: lower case
remove


http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@98
PS1, Line 98: lower case
remove


http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@99
PS1, Line 99: lower case
remove



-- 
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensou...@gmail.com
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 15 Nov 2017 18:22:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 6:

Based on the conversation on dev@ 
(https://lists.apache.org/thread.html/bf1cbd4644a18489f99ee708291b348cef9a379a26d5ebe6d043d3f2@%3Cdev.impala.apache.org%3E),
 I suggest we wait on this until 3.0.


--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 15 Nov 2017 15:58:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 13: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 15 Nov 2017 15:54:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8543 )

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..


Patch Set 2:

I'm testing this here:

https://jenkins.impala.io/job/gerrit-verify-dryrun/1471/console

If it passes, it won't +Verify (it is on dry-run setting), but I'll be 
comfortable with +1 Code-Review after that. I'll let Alex +2.


--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 15 Nov 2017 15:50:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8510 )

Change subject: IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow
..


Patch Set 2:

Sailesh, you're the person my intuition leads me to ask about encryption modes. 
Can you take a look?


--
To view, visit http://gerrit.cloudera.org:8080/8510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib97939f2334838263364b53ef3413871638bf53e
Gerrit-Change-Number: 8510
Gerrit-PatchSet: 2
Gerrit-Owner: Xianda Ke 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 15 Nov 2017 15:47:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc@179
PS6, Line 179:   thread_local std::uniform_real_distribution 
distribution(min, max);
> You're right. I think thread_local is better than static to avoid the guard
I'm not confident that works nicely with boost::thread and pthreads. I emailed 
dev@.

I'd suggest no storage modifier at all in this patch, getting it committed, 
then revisiting if useful for once the semantics are clearer. In this case, it 
appears that std::uniform_real_distribution does very little initialization 
code and so it has very little performance penalty to run it in every execution.



-- 
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 17:32:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6160: Allow multiple statements in a Query object.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8513 )

Change subject: IMPALA-6160: Allow multiple statements in a Query object.
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8513/4/tests/performance/query_executor.py
File tests/performance/query_executor.py:

http://gerrit.cloudera.org:8080/#/c/8513/4/tests/performance/query_executor.py@45
PS4, Line 45: DDL/DML
nit: CRUD


http://gerrit.cloudera.org:8080/#/c/8513/4/tests/performance/query_executor.py@190
PS4, Line 190: query (Query)
nit: can you please pluralize query everywhere it can be multiple queries, 
including this comment, line 200, the argument in line 206, the attribute on 
line 209, and so on?


http://gerrit.cloudera.org:8080/#/c/8513/4/tests/performance/query_executor.py@230
PS4, Line 230: (generally, DDL and DML statements)
nit: please remove



--
To view, visit http://gerrit.cloudera.org:8080/8513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Gerrit-Change-Number: 8513
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Tue, 14 Nov 2017 14:55:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8543 )

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..


Patch Set 1:

> (1 comment)
 >
 > I would like to add a test which contains wrong row-size value and
 > the test should show that an error always happen. Unfortunately, I
 > haven't found any similar case from testdata. I thought JUnit test,
 > but E2E test should be required in this case.

In the walkthrough 
(https://lists.apache.org/thread.html/116ce61e65bfbb8ecbc7bd128609fd4e23d44385fe6357ed066b9818@%3Cdev.impala.apache.org%3E),
 I wrote "Once that's done, try running the Planner tests on a test that 
includes row-size again. This time, it should pass with the row-size as written 
and fail if you change the row-size, like we did above."

Did you try that?


--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 14:32:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc@179
PS6, Line 179:   static std::uniform_real_distribution 
distribution(min, max);
For non-PODs like std::uniform_real_distribution, you probably want to avoid 
static, since it can cause synchronization and slowness.

See the guard variable: https://godbolt.org/g/9BuKd6



-- 
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 14:29:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h
File be/src/util/rand-util.h:

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h@28
PS3, Line 28:   static double generate(std::mt19937& generator, const double 
min, const double max);
> It will not necessarily be possible or wise to replace all IMPALA-4954 uses
Thanks for adding the e2e tests. I'd still recommend the file removals 
referenced in this comment.



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 05:25:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Pin gen build version's git handling to typical git dir.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8500 )

Change subject: Pin gen_build_version's git handling to typical git dir.
..


Patch Set 1:

> Sounds like https://jira.cloudera.com/browse/RELENG-2864

Zach, this is the public ASF Impala repo. That JIRA is not visible to all 
Impala contributors, only those who work at Cloudera.

Apache Impala should be as inclusive as possible, and not work in public on 
patches in a way only Clouderans can view.

Thanks!


--
To view, visit http://gerrit.cloudera.org:8080/8500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I98870a1cb2073ddcf9ba1620e3801e1964930edd
Gerrit-Change-Number: 8500
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:31:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6160: Allow multiple statements in a Query object.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8513 )

Change subject: IMPALA-6160: Allow multiple statements in a Query object.
..


Patch Set 3:

(2 comments)

Just nits left

http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py
File tests/performance/query_executor.py:

http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@52
PS1, Line 52:
> I consider SELECT as DML (data manipulation, but not mutation. :)  I could
I get that you consider SELECT as DML, but I don't think most other people do, 
so I'd suggest renaming as a favor to them.


http://gerrit.cloudera.org:8080/#/c/8513/3/tests/performance/query_executor.py
File tests/performance/query_executor.py:

http://gerrit.cloudera.org:8080/#/c/8513/3/tests/performance/query_executor.py@190
PS3, Line 190: query (Query): SQL query (batch, ;-delimited) to be executed
This is still queries, not a single query, right?



--
To view, visit http://gerrit.cloudera.org:8080/8513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Gerrit-Change-Number: 8513
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Mon, 13 Nov 2017 20:53:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [DOCS] Remove unnecessary 'incubator' from URLs

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8524 )

Change subject: [DOCS] Remove unnecessary 'incubator' from URLs
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8524/1/docs/topics/impala_langref_unsupported.xml
File docs/topics/impala_langref_unsupported.xml:

http://gerrit.cloudera.org:8080/#/c/8524/1/docs/topics/impala_langref_unsupported.xml@207
PS1, Line 207: Apache
> Might as well shorten this opening tag to . That way
Done



--
To view, visit http://gerrit.cloudera.org:8080/8524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0f8d51b147094e629e60c4a8c5aecbb6cdb6a8e
Gerrit-Change-Number: 8524
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-Reviewer: Michael Brown 
Gerrit-Comment-Date: Sat, 11 Nov 2017 21:45:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [DOCS] Remove unnecessary 'incubator' from URLs

[Jim Apple (Code Review)]

Hello Michael Brown, John Russell, Laurel Hale,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8524

to look at the new patch set (#2).

Change subject: [DOCS] Remove unnecessary 'incubator' from URLs
..

[DOCS] Remove unnecessary 'incubator' from URLs

URLs like https://impala.incubator.apache.org/ are aliases with
https://impala.apache.org/, so we can use the latter and avoid making
any changes if or when Impala graduates from the incubator.

Change-Id: If0f8d51b147094e629e60c4a8c5aecbb6cdb6a8e
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_langref_unsupported.xml
2 files changed, 5 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/8524/2
--
To view, visit http://gerrit.cloudera.org:8080/8524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0f8d51b147094e629e60c4a8c5aecbb6cdb6a8e
Gerrit-Change-Number: 8524
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] [DOCS] Remove unnecessary 'incubator' from URLs

[Jim Apple (Code Review)]

Jim Apple has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8524


Change subject: [DOCS] Remove unnecessary 'incubator' from URLs
..

[DOCS] Remove unnecessary 'incubator' from URLs

URLs like https://impala.incubator.apache.org/ are aliases with
https://impala.apache.org/, so we can use the latter and avoid making
any changes if or when Impala graduates from the incubator.

Change-Id: If0f8d51b147094e629e60c4a8c5aecbb6cdb6a8e
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_langref_unsupported.xml
2 files changed, 5 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/8524/1
--
To view, visit http://gerrit.cloudera.org:8080/8524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0f8d51b147094e629e60c4a8c5aecbb6cdb6a8e
Gerrit-Change-Number: 8524
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] IMPALA-6160: Rework query execution to handle multiple statements in a Query object.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8513 )

Change subject: IMPALA-6160: Rework query execution to handle multiple 
statements in a Query object.
..


Patch Set 1:

(9 comments)

Thanks for taking this on!

http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-6160: Rework query execution to handle multiple statements 
in a Query object.
nit: For the subject line, please keep it under 72 characters and one line. 
Maybe "Allow multiple statements in a query object"?


http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG@14
PS1, Line 14:   --query_iterations=3 --table_formats=parquet/none --plan_first 
--query_names='.*'
nit: can you wrap at 72 characters, here? If you use emacs to write your commit 
message, you can wrap automatically with ctrl-q, though it might not understand 
your bullets and then rewrap those unhelpfully.


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py
File tests/performance/query_executor.py:

http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@45
PS1, Line 45: query options' names
I'm a bit confused about this - I don't see query options in these regexes.


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@52
PS1, Line 52: SELECT
SELECT starts statements that are not DDL or DML, yes?


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@52
PS1, Line 52: re.I
I think spelling out IGNORECASE makes it much more readable.


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@186
PS1, Line 186:   """Executes a query.
Can you change the comments and member variable to pluralize "query"?


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@224
PS1, Line 224: a set of SQL statements
This looks very general to me. Might you be able to get by with something 
simpler that only handles one SET followed by one EXPLAINable statement?


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@229
PS1, Line 229: timing
How DOES the timing work for this object? There doesn't seem to be any timing 
code directly in this. Maybe it is done in self.exec_func storing the timing 
info somewhere? The EXPLAINs are done with self.exec_func as well, so I'd guess 
that every call to self.exec_func clobbers the old timing. If that's right, 
then executing multiple queries will lead to only a timing from the last call 
being stored, yes?


http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@231
PS1, Line 231:
 : This function originally assumed that self.query contained 
only a single
 : query statement, but that assumption is not valid for a test 
file that
 : contains, e.g., a SET DECIMAL_V2=n; statement before the 
actual query for the
 : test.
nit: I think you can omit this.



--
To view, visit http://gerrit.cloudera.org:8080/8513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297
Gerrit-Change-Number: 8513
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Sat, 11 Nov 2017 04:02:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@307
PS2, Line 307: if ! curl "${CURL_ARGS[@]}" ; then
curl is not a development dependency in bin/bootstrap_system.sh. I'd suggest 
using wget or adding curl to the apt-get install list in bin/bootstrap_system.sh



--
To view, visit http://gerrit.cloudera.org:8080/8294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 08 Nov 2017 18:20:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2281: Replace FNV with FastHash in exchange nodes

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8417 )

Change subject: IMPALA-2281: Replace FNV with FastHash in exchange nodes
..


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8417/5/be/src/util/hash-util.h
File be/src/util/hash-util.h:

http://gerrit.cloudera.org:8080/#/c/8417/5/be/src/util/hash-util.h@32
PS5, Line 32:  public:
> Done. I followed the practice in be/src/kudu/security/init.cc and assumed t
I'm not 100% on that, since http://www.apache.org/legal/src-headers.html#3party 
says, "Do not modify or remove any copyright notices or licenses within 
third-party works," but if Kudu does it, it's probably the right thing to do.



--
To view, visit http://gerrit.cloudera.org:8080/8417
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I778317d982dcdb94173a369a65b39f32b4f7ded2
Gerrit-Change-Number: 8417
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 08 Nov 2017 04:17:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6105: Clarify argument order in single node perf run

[Jim Apple (Code Review)]

Jim Apple has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8470 )

Change subject: IMPALA-6105: Clarify argument order in single_node_perf_run
..

IMPALA-6105: Clarify argument order in single_node_perf_run

single_node_perf_run.py uses git_hash_A vs. git_hash_B, distinguish
them by their position in the command-line
arguments. single_node_perf_run.py calls report_benchmark_results.py,
which uses the "reference vs. input", distinguished by their
command-line flags. The output of report_benchmark_results.py uses
"{empty string} vs Base".

In the long run, I think it would be better to fix all three to use
the same terminology, but this comment hopefully adds clarity.

Change-Id: Ib236ce7e83dc193ef1382f630ce58759a639
Reviewed-on: http://gerrit.cloudera.org:8080/8470
Tested-by: Impala Public Jenkins
Reviewed-by: Jim Apple 
---
M bin/single_node_perf_run.py
1 file changed, 22 insertions(+), 2 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Jim Apple: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/8470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib236ce7e83dc193ef1382f630ce58759a639
Gerrit-Change-Number: 8470
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6105: Clarify argument order in single node perf run

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8470 )

Change subject: IMPALA-6105: Clarify argument order in single_node_perf_run
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib236ce7e83dc193ef1382f630ce58759a639
Gerrit-Change-Number: 8470
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 07 Nov 2017 16:16:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 3:

(7 comments)

Thanks for sending the patch. The core change looks good to me; I have some 
suggestions on the testing and the commit message

http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@11
PS3, Line 11: in C++11 libarary
"in the C++11 standard library"


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@12
PS3, Line 12: because it has much longer period than that of rand in C.
It does show better randomness, and it does have a longer period, but it 
doesn't show better randomness just because it has a longer period. I would 
remove this clause.


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@13
PS3, Line 13: (More details in http://www.pcg-random.org/)
That's a site promoting a different generator. If you want to list a citation, 
I would use "Mersenne twister: a 623-dimensionally equidistributed uniform 
pseudo-random number generator."


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@17
PS3, Line 17: t1
This is not a reproducable test case without knowing what t1 is. You can use 
the tables in functional to simulate this, maybe by using a cross join.


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@23
PS3, Line 23:
: * After
: > select count(distinct(rand(1))), count(*) from t1
: +---+---+
: | count(distinct (rand(1))) | count(*)  |
: +---+---+
: | 34603008  | 103809024 |
: +---+---+
:
: You may expect maximum randomness(e.g. 103809024).
: Due to the issue IMPALA-6117, randomness could be
: "maximum randomess / n". "n" means the number of Impala
: execution engines. n is 3 in this example and each
: execution engine loads and processes data in parallel.
:
: This change introduces a new utility code for random because
: we have a plan to replace the legacy in IMPALA-4954 with
: the utility code.
I'd say this whole section is overkill.


http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/exprs/expr-test.cc@4652
PS3, Line 4652:   {
You can reduce this code size and make it more robust to future changes to 
Impala's rand() algorithm in two ways:

1. Make it a short loop:

for (uint32_t seed : {0, 1234}) {

2. Instead of pre-calculating the expected value, in the first call to Impala's 
rand(), use GetValue("rand()", TYPE_DOUBLE). Save that value and use it as the 
expected value in the second call to "rand()" and the call to "random()".


http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h
File be/src/util/rand-util.h:

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h@28
PS3, Line 28:   static double generate(std::mt19937& generator, const double 
min, const double max);
It will not necessarily be possible or wise to replace all IMPALA-4954 uses 
with this code. I'd recommend doing away with this header, the .cc file, and 
the standalone test .cc file.

Instead, I'd suggest some end-to-end tests.

The trickiest one will probably be the distinct test, which can very based on 
the number of nodes. Take a look at the method test_distinct_estimate to see 
how to force the query to run with a single node. The file it references is 
distinct-estimate.test - you can see the .test file format from that file.

I would recommend a new test file just for testing rand.



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 07 Nov 2017 04:40:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6105: Clarify argument order in single node perf run

[Jim Apple (Code Review)]

Jim Apple has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8470


Change subject: IMPALA-6105: Clarify argument order in single_node_perf_run
..

IMPALA-6105: Clarify argument order in single_node_perf_run

single_node_perf_run.py uses git_hash_A vs. git_hash_B, distinguish
them by their position in the command-line
arguments. single_node_perf_run.py calls report_benchmark_results.py,
which uses the "reference vs. input", distinguished by their
command-line flags. The output of report_benchmark_results.py uses
"{empty string} vs Base".

In the long run, I think it would be better to fix all three to use
the same terminology, but this comment hopefully adds clarity.

Change-Id: Ib236ce7e83dc193ef1382f630ce58759a639
---
M bin/single_node_perf_run.py
1 file changed, 22 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8470/1
--
To view, visit http://gerrit.cloudera.org:8080/8470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib236ce7e83dc193ef1382f630ce58759a639
Gerrit-Change-Number: 8470
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] Install OpenJDK-dbg for development environments.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8431 )

Change subject: Install OpenJDK-dbg for development environments.
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8431
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0
Gerrit-Change-Number: 8431
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 03 Nov 2017 03:30:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] Install OpenJDK-dbg for development environments.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8431 )

Change subject: Install OpenJDK-dbg for development environments.
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8431
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0
Gerrit-Change-Number: 8431
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 03 Nov 2017 03:13:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc@6037
PS4, Line 6037: TYPE_BIGINT, 28123);
> This is a compatibility breaking change; we need to document the issue.
I would suggest discussing this on dev@, since some other compat-breaking 
changes have been pushed to 3.0 and some other compat-breaking changes have 
been hidden behind a feature flag.



--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 31 Oct 2017 14:41:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5243: Speed up code gen for wide Avro tables.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8211 )

Change subject: IMPALA-5243: Speed up code gen for wide Avro tables.
..


Patch Set 8:

> Oof. GVO caught a (correct) clang-tidy issue. Alas, clang-tidy is
 > not in pre-review-test, so this was my first experience with that.

You can also use 
https://jenkins.impala.io/view/Utility/job/gerrit-verify-dryrun-external/ , 
which does run clang-tidy.


--
To view, visit http://gerrit.cloudera.org:8080/8211
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f1b390be4adf6e6699a18344234f8ff7ee74476
Gerrit-Change-Number: 8211
Gerrit-PatchSet: 8
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Oct 2017 21:32:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 13: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 26 Oct 2017 13:25:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:

http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh@111
PS12, Line 111: # Install these as sudo so that we can avoid the interactive 
dialog screens.
The apt-get on line 106 is calling the function on line 79. Maybe the problem 
with the env var was the sudo pass-through blockage on line 82? What if you 
added DF=ni on that line.

Maybe also gate it on ther terminal interactivity, like on line 41?



--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 26 Oct 2017 05:26:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Remove Parquet "incubating" modifier in README

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8378 )

Change subject: Remove Parquet "incubating" modifier in README
..


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c261f32aa90aec67dbd1e5f172323e4c3feaa1d
Gerrit-Change-Number: 8378
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 25 Oct 2017 15:51:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] fix doc about parquet under incubation

[Jim Apple (Code Review)]

Jim Apple has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8378


Change subject: fix doc about parquet under incubation
..

fix doc about parquet under incubation

Change-Id: I2c261f32aa90aec67dbd1e5f172323e4c3feaa1d
---
M README.md
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/8378/1
--
To view, visit http://gerrit.cloudera.org:8080/8378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c261f32aa90aec67dbd1e5f172323e4c3feaa1d
Gerrit-Change-Number: 8378
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] Remove Parquet "incubating" modifier in README

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8378 )

Change subject: Remove Parquet "incubating" modifier in README
..

Remove Parquet "incubating" modifier in README

Change-Id: I2c261f32aa90aec67dbd1e5f172323e4c3feaa1d
---
M README.md
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/8378/2
--
To view, visit http://gerrit.cloudera.org:8080/8378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c261f32aa90aec67dbd1e5f172323e4c3feaa1d
Gerrit-Change-Number: 8378
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR](asf-site) Fix broken link to CIDR paper

[Jim Apple (Code Review)]

Jim Apple has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8365 )

Change subject: Fix broken link to CIDR paper
..

Fix broken link to CIDR paper

Change-Id: I80f222465803e8c8539541df6f11d8c7808aa335
Reviewed-on: http://gerrit.cloudera.org:8080/8365
Reviewed-by: Bharath Vissapragada 
Tested-by: Jim Apple 
---
M overview.html
1 file changed, 3 insertions(+), 1 deletion(-)

Approvals:
  Bharath Vissapragada: Looks good to me, approved
  Jim Apple: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: merged
Gerrit-Change-Id: I80f222465803e8c8539541df6f11d8c7808aa335
Gerrit-Change-Number: 8365
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR](asf-site) Fix broken link to CIDR paper

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8365 )

Change subject: Fix broken link to CIDR paper
..


Patch Set 1: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I80f222465803e8c8539541df6f11d8c7808aa335
Gerrit-Change-Number: 8365
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Tue, 24 Oct 2017 06:15:30 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Fix broken link to CIDR paper

[Jim Apple (Code Review)]

Jim Apple has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8365


Change subject: Fix broken link to CIDR paper
..

Fix broken link to CIDR paper

Change-Id: I80f222465803e8c8539541df6f11d8c7808aa335
---
M overview.html
1 file changed, 3 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/8365/1
--
To view, visit http://gerrit.cloudera.org:8080/8365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: newchange
Gerrit-Change-Id: I80f222465803e8c8539541df6f11d8c7808aa335
Gerrit-Change-Number: 8365
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] IMPALA-6070: Parallel data load.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8320 )

Change subject: IMPALA-6070: Parallel data load.
..


Patch Set 2: Code-Review+1

LGTM. not +2ing so others have a chance to weigh in as to whether you have 
addressed their comments.


--
To view, visit http://gerrit.cloudera.org:8080/8320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836c4e1586f229621c102c4f4ba22ce7224ab9ac
Gerrit-Change-Number: 8320
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Sat, 21 Oct 2017 22:23:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6073: Fail on misconfigured CLASSPATH.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8327 )

Change subject: IMPALA-6073: Fail on misconfigured CLASSPATH.
..


Patch Set 1:

> I think this check seems not to be enough. Let me leave a detailed
 > comment on the JIRA ticket.

It is OK to leave detailed comments here in the code review tool. In fact, it 
is common.


--
To view, visit http://gerrit.cloudera.org:8080/8327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb92fa5cad8bbc29b620bec5904e45ed7aeff13d
Gerrit-Change-Number: 8327
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 19 Oct 2017 10:39:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6070: Parallel data load.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8320 )

Change subject: IMPALA-6070: Parallel data load.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-hive-server.sh
File testdata/bin/run-hive-server.sh:

http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-hive-server.sh@75
PS1, Line 75:   HADOOP_HEAPSIZE="1024" hive --service hiveserver2 > 
${LOGDIR}/hive-server2.out 2>&1 &
> I'd prefer to keep this change. Our Hive server tends to OOM pretty easily
People like Alex are those whom I was most concerned about, as I know he used 
to develop Impala on a machine without much memory.

If Alex is OK with this, I am, too.



--
To view, visit http://gerrit.cloudera.org:8080/8320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836c4e1586f229621c102c4f4ba22ce7224ab9ac
Gerrit-Change-Number: 8320
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 19 Oct 2017 00:09:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6070: Parallel data load.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8320 )

Change subject: IMPALA-6070: Parallel data load.
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8320/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8320/1//COMMIT_MSG@9
PS1, Line 9: This commit loads functional-query, TPC-H data, and TPC-DS data in 
parallel. In
nit: Can you wrap this at the red line provided by gerrit? I think it is 72 
characters. Emacs will wrap it for you at the right space with ctrl-q, if you 
choose.


http://gerrit.cloudera.org:8080/#/c/8320/1//COMMIT_MSG@12
PS1, Line 12: minuites
nit: minutes


http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/create-load-data.sh@480
PS1, Line 480:   run-step-backgroundable "Loading functional-query data" 
load-functional-query.log \
Could add a comment about what you decided to background and what you decided 
not to, and why?


http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-hive-server.sh
File testdata/bin/run-hive-server.sh:

http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-hive-server.sh@75
PS1, Line 75:   HADOOP_HEAPSIZE="1024" hive --service hiveserver2 > 
${LOGDIR}/hive-server2.out 2>&1 &
> I'm currently testing to see if 512 is enough.
This looks like it will also increase HADOOP_HEAPSIZE when not doing a parallel 
load, which is a shame. Do you see a way around that?


http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-step.sh
File testdata/bin/run-step.sh:

http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-step.sh@53
PS1, Line 53:
nit: only one empty line, to match context


http://gerrit.cloudera.org:8080/#/c/8320/1/testdata/bin/run-step.sh@84
PS1, Line 84:   RUN_STEP_PIDS=()
Do you want to reset MSGS, too?



--
To view, visit http://gerrit.cloudera.org:8080/8320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836c4e1586f229621c102c4f4ba22ce7224ab9ac
Gerrit-Change-Number: 8320
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 18 Oct 2017 23:17:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@276
PS1, Line 276: 169.254.169.254
What is this address? Can we use a domain name and https?

What would a new Impala developer get as a result of this?



--
To view, visit http://gerrit.cloudera.org:8080/8294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 18 Oct 2017 20:55:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Add 'psmisc' to bootstrap system.sh.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8306 )

Change subject: Add 'psmisc' to bootstrap_system.sh.
..


Patch Set 1:

Would you like me to submit this for you, Phil?


--
To view, visit http://gerrit.cloudera.org:8080/8306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I378b6fbf7a7b3f0b42ecf064e98cfe6e29b3
Gerrit-Change-Number: 8306
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 18 Oct 2017 17:09:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] Add 'psmisc' to bootstrap system.sh.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8306 )

Change subject: Add 'psmisc' to bootstrap_system.sh.
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I378b6fbf7a7b3f0b42ecf064e98cfe6e29b3
Gerrit-Change-Number: 8306
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 18 Oct 2017 04:44:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8262 )

Change subject: IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
..


Patch Set 3:

> Sorry to get to this a bit late.

No apology required.

I agree with all of your suggestions.

If you want to file a ticket and assign it to me or if you want to send me a 
patch to review, I'd be amenable.


--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 17 Oct 2017 05:22:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] Making bin/bootstrap system.sh executable.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8292 )

Change subject: Making bin/bootstrap_system.sh executable.
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15973f68e0d6cba86da58a104a607cac0627c4cb
Gerrit-Change-Number: 8292
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 17 Oct 2017 05:23:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8262 )

Change subject: IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
..


Patch Set 2: Verified+1

Verified in https://jenkins.impala.io/job/gerrit-verify-dryrun/1334/


--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Oct 2017 20:54:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8262 )

Change subject: IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
..


Patch Set 2:

Passed GVD: https://jenkins.impala.io/job/gerrit-verify-dryrun/1334/


--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Oct 2017 20:27:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04

[Jim Apple (Code Review)]

Hello Lars Volker, Michael Brown, Zoram Thanga,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8262

to look at the new patch set (#2).

Change subject: IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
..

IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04

This commit bundles two changes.

The first extracts from bootstrap_development.sh the commands to
prepare a system to build-and-test without actually doing so. This
enables custom build commands that might not load the test data, for
instance.

The second changes bootstrap_build.sh to work with Ubuntu 16.04. It
should still work with Ubuntu 14.04, but I don't anticipate that being
part of the Jenkins pre-merge job anymore, so I removed that from the
comment at the top of the file explaining what it does.

Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
---
M bin/bootstrap_build.sh
M bin/bootstrap_development.sh
A bin/bootstrap_system.sh
3 files changed, 227 insertions(+), 174 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8262/2
--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] Make build scripts more friendly to Ubuntu 16.04

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8262 )

Change subject: Make build scripts more friendly to Ubuntu 16.04
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8262/1/bin/bootstrap_build.sh
File bin/bootstrap_build.sh:

http://gerrit.cloudera.org:8080/#/c/8262/1/bin/bootstrap_build.sh@23
PS1, Line 23: # 1. At least 8GB of free disk space
> Is this still true? I vaguely recollect running out of space in a vm that h
Yes, check the df output: 
https://jenkins.impala.io/view/Experimental/job/ubuntu-16.04-build-only/4/consoleFull

You may have been loading data, building static (and building all tests), or 
other developer activities.



--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Oct 2017 05:00:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5429: Multi threaded block metadata loading

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8235 )

Change subject: IMPALA-5429: Multi threaded block metadata loading
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8235/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8235/4//COMMIT_MSG@59
PS4, Line 59:  100K-PARTITIONS-1M-FILES-CUSTOM-11-REFRESH-PARTITION I -16.4%
> Thanks for pointing this out. I should've added more context here. Mostafa
I see. So are these literally as simple as, to pick the first one, a single 
refresh call?



--
To view, visit http://gerrit.cloudera.org:8080/8235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I07eaa7151dfc4d56da8db8c2654bd65d8f808481
Gerrit-Change-Number: 8235
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 11 Oct 2017 01:14:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5429: Multi threaded block metadata loading

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8235 )

Change subject: IMPALA-5429: Multi threaded block metadata loading
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8235/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8235/4//COMMIT_MSG@59
PS4, Line 59:  100K-PARTITIONS-1M-FILES-CUSTOM-11-REFRESH-PARTITION I -16.4%
What are these benchmarks?



--
To view, visit http://gerrit.cloudera.org:8080/8235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I07eaa7151dfc4d56da8db8c2654bd65d8f808481
Gerrit-Change-Number: 8235
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 10 Oct 2017 03:48:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Remove old leftover, unmaintained parts of website.

[Jim Apple (Code Review)]

Jim Apple has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8234 )

Change subject: Remove old leftover, unmaintained parts of website.
..

Remove old leftover, unmaintained parts of website.

Change-Id: Ia437d37c40d6cc2c8491cc9695f7ce27501b6797
Reviewed-on: http://gerrit.cloudera.org:8080/8234
Reviewed-by: David Knupp 
Tested-by: Jim Apple 
---
M bylaws.html
M community.html
M downloads.html
M impala-docs.html
M index.html
M overview.html
6 files changed, 0 insertions(+), 109 deletions(-)

Approvals:
  David Knupp: Looks good to me, approved
  Jim Apple: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia437d37c40d6cc2c8491cc9695f7ce27501b6797
Gerrit-Change-Number: 8234
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR](asf-site) Remove old leftover, unmaintained parts of website.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8234 )

Change subject: Remove old leftover, unmaintained parts of website.
..


Patch Set 1: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia437d37c40d6cc2c8491cc9695f7ce27501b6797
Gerrit-Change-Number: 8234
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 10 Oct 2017 00:29:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7438 )

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7438/11/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/7438/11/be/src/exprs/expr-test.cc@1544
PS11, Line 1544: DCHECK(false) << "Unexpected character.";
This is causing a clang-tidy error in your last GVD:

See the last lines of 
https://jenkins.impala.io/job/gerrit-verify-dryrun/1292/console

ERROR: There were some failed jobs: 
[https://jenkins.impala.io/job/clang-tidy/1606/, 
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/374/]

Then the last lines of the URL it points to, 
https://jenkins.impala.io/job/clang-tidy/1606/console:

/home/ubuntu/Impala/be/src/exprs/expr-test.cc:1543:7: warning: variable 
'digit' is used uninitialized whenever switch default is taken 
[clang-diagnostic-sometimes-uninitialized]



--
To view, visit http://gerrit.cloudera.org:8080/7438
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-Change-Number: 7438
Gerrit-PatchSet: 11
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 04 Oct 2017 03:05:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) A blog post about IMPALA-4916

[Jim Apple (Code Review)]

Jim Apple has abandoned this change. ( http://gerrit.cloudera.org:8080/5995 )

Change subject: A blog post about IMPALA-4916
..


Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/5995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ifa90bf5621ef6466a4821f77a6e8a8b20c5512ae
Gerrit-Change-Number: 5995
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7438 )

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 6:

> > I totally agree about doing anything fancy as a followup.  In
 > > particular, I did not find anything at all useful in AVX to help.
 > > There is no vectored multiply large enough for this, and FMA
 > > operations won't help either as they basically only help with
 > > precision loss on floating point types.
 > >
 > > Considering the perf difference is so extreme in some cases, I
 > > think we should strongly consider either living with the broken
 > > behavior until we can come up with a solution, or else verifying
 > > with users making use of the DECIMAL_V2 feature that this will
 > not
 > > be a problem.
 >
 > AVX2 has 32x8 times 32x8 multiplication. I have elsewhere simulated
 > 64-bit multiplication with this, combines with additions and
 > shifts, and received a speedup on 64x4 times 64x4 operations.

Oh, and AVX512 has vpmullq for multiplying vectors of 64-bit values.


--
To view, visit http://gerrit.cloudera.org:8080/7438
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-Change-Number: 7438
Gerrit-PatchSet: 6
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 28 Sep 2017 01:56:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7438 )

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 6:

> I totally agree about doing anything fancy as a followup.  In
 > particular, I did not find anything at all useful in AVX to help.
 > There is no vectored multiply large enough for this, and FMA
 > operations won't help either as they basically only help with
 > precision loss on floating point types.
 >
 > Considering the perf difference is so extreme in some cases, I
 > think we should strongly consider either living with the broken
 > behavior until we can come up with a solution, or else verifying
 > with users making use of the DECIMAL_V2 feature that this will not
 > be a problem.

AVX2 has 32x8 times 32x8 multiplication. I have elsewhere simulated 64-bit 
multiplication with this, combines with additions and shifts, and received a 
speedup on 64x4 times 64x4 operations.


--
To view, visit http://gerrit.cloudera.org:8080/7438
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-Change-Number: 7438
Gerrit-PatchSet: 6
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 27 Sep 2017 20:31:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7974/9/.clang-tidy
File .clang-tidy:

Line 27: -clang-analyzer-cplusplus.NewDeleteLeaks,\
I'm concerned that some of these went in with only self-review. Some of them, 
by their name, look very useful to me. What is your preferred forum to discuss 
those new warning suppressions?


-- 
To view, visit http://gerrit.cloudera.org:8080/7974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida873ddb15e393b0bd37486db24add8a32f43ad0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:

Line 135:   void InsertNoAvx(const uint32_t hash) noexcept;
naming nit: We actually need AVX2 -- AVX won't cut it


-- 
To view, visit http://gerrit.cloudera.org:8080/8029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Change log for 2.10.0 release

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Change log for 2.10.0 release
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d47cf805205b25861e38d106412bb7f892016a0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8073
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3072eda6f34a5e9245ddec70f725a65a13a14b78
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Revert "Update download and signature links for 2.10.0 release."

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Revert "Update download and signature links for 2.10.0 release."
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I592b22771a9b345448887c129eacfe4d88d7e59d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/2/downloads.html
File downloads.html:

PS2, Line 163: 1
512


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8052/1/downloads.html
File downloads.html:

Line 162: 
"https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incubating-2.10.0.tar.gz.sha;>
404. 
https://www.apache.org/dist/incubator/impala/2.10.0/apache-impala-incubating-2.10.0.tar.gz.sha512
 ?


-- 
To view, visit http://gerrit.cloudera.org:8080/8052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5905: add script for all-build-options job
..


Patch Set 2: Code-Review+2

(3 comments)

Can you add a note about ccache and ninja?

http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:

Line 1
> Yeah the jenkins/ subdirectory makes sense to me, there's a lot of junk in 
I named the Jenkins job "all-build-options". I'm OK with a name change of any 
sort, or leaving it the same. Maybe "build-with-all-flag-combinations.sh"?


Line 21
Required the ninja build system and ccache to be installed, which are not 
strictly build requirements.


PS1, Line 35: 
: 
: 
: 
: 
: 
: 
> I just preserved this logic from the original Jenkins job script. I didn't 
One difference: if clean.sh fails in the call to buildall, that call fails but 
we continue to test more build options. This way, if we can't clean, we just 
dies since all the remaining calls to buildall.sh should also fail but will 
provide no additional information.


-- 
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2107: [DOCS] Document base64*code() functions

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2107: [DOCS] Document base64*code() functions
..


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7963
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5251e368ad36756c19a7b97e5ef6f232f616189b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5902: add ThreadSanitizer build

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5902: add ThreadSanitizer build
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7977/1/tests/common/environ.py
File tests/common/environ.py:

Line 135: either compiled with code coverage enabled, ASAN or TSAN.
> I wasn't sure if UBSAN was just missing from this file - is it just treated
I suspect so.


-- 
To view, visit http://gerrit.cloudera.org:8080/7977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I22f8faeefa5e157279c5973fe28bc573b7606d50
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5902: add ThreadSanitizer build

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5902: add ThreadSanitizer build
..


Patch Set 1:

(8 comments)

Nice, thanks for taking this on.

Can you file a ticket to add this to 
https://jenkins.impala.io/job/all-build-options/ when this is done?

http://gerrit.cloudera.org:8080/#/c/7977/1/be/src/common/init.cc
File be/src/common/init.cc:

Line 140
No longer needed? Why not?


Line 199: #ifndef THREAD_SANITIZER
Why can't we run this with TSAN? Should the #ifndef go inside that function 
itself?


http://gerrit.cloudera.org:8080/#/c/7977/1/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

Line 115:   // tcmalloc and address sanitizer cannot be used together
nit: "or thread"


http://gerrit.cloudera.org:8080/#/c/7977/1/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:

Line 139:   ss << "Memory tracking is not available with address sanitizer 
builds.";
nit: "or thread"


http://gerrit.cloudera.org:8080/#/c/7977/1/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:

PS1, Line 86: asan
naming?


http://gerrit.cloudera.org:8080/#/c/7977/1/be/src/util/memory-metrics.h
File be/src/util/memory-metrics.h:

Line 136: /// Alternative to TCMallocMetric if we're running under Address 
Sanitizer, which
Naming?


http://gerrit.cloudera.org:8080/#/c/7977/1/bin/start-impalad.sh
File bin/start-impalad.sh:

PS1, Line 104: 7
How did you pick 7?


http://gerrit.cloudera.org:8080/#/c/7977/1/tests/common/environ.py
File tests/common/environ.py:

Line 135: either compiled with code coverage enabled or a sanitizer.
except ubsan


-- 
To view, visit http://gerrit.cloudera.org:8080/7977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I22f8faeefa5e157279c5973fe28bc573b7606d50
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2107: [DOCS] Document base64*code() functions

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2107: [DOCS] Document base64*code() functions
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7963/1/docs/shared/impala_common.xml
File docs/shared/impala_common.xml:

Line 802: MAX(), and MIN() than when
Are you sure about MAX and MIN? They might preserve ordering. I don't know.


PS1, Line 808: All argument values
 : supplied to base64encode() must also be 
a
 : multiple of 4 bytes in length.
I don't think this is right. The example you give below, for instance, is 
'hello world', which is 11 characters.

Did you mean that all arguments to base64decode must be a multiple of 4 bytes 
in length?


http://gerrit.cloudera.org:8080/#/c/7963/1/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:

Line 88:   
This was fixed in 2.6.0, according to the ticket, not 2.9.0


-- 
To view, visit http://gerrit.cloudera.org:8080/7963
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5251e368ad36756c19a7b97e5ef6f232f616189b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5366: datetime missing when upgrading to Ubuntu 16.04

[Jim Apple (Code Review)]

Jim Apple has abandoned this change.

Change subject: IMPALA-5366: datetime missing when upgrading to Ubuntu 16.04
..


Abandoned

-- 
To view, visit http://gerrit.cloudera.org:8080/6989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I41501d273967dc1d57d366a145766a67b2dbbaa0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5764: Allow overriding packaged components
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export 
DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> The proprietary stuff is what I'm actually trying to get rid of with this c
Got it. However, my comment was trying to address the jargon of your comment, 
not the goal of the patch. I don't think there is any problem with the goal of 
the patch.


-- 
To view, visit http://gerrit.cloudera.org:8080/7581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 6: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 6:

Thanks for fixing those last comments! I'm starting the pre-merge job now; it 
takes roughly four hours, and it will commit this patch if it passes all of the 
tests.

-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 17:

> That build included this patch. Or do you have reason to believe it
 > didn't?

Ah, OK. That's a part of Gerrit I have not yet understood; thanks for the 
clarification!

-- 
To view, visit http://gerrit.cloudera.org:8080/5716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 17:

> Verified by build: https://jenkins.impala.io/job/gerrit-verify-dryrun/1051/

I'm confused. That build verified https://gerrit.cloudera.org/#/c/5717/19

-- 
To view, visit http://gerrit.cloudera.org:8080/5716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5764: Allow overriding packaged components
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export 
DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> Technically this "breaks" the C6 build, but because the cdh6.x version of I
This is the code review tool for Apache Impala. It is fine to add features to 
Impala to help other software that works with Impala, including proprietary 
products.

However, please try to explain enough in your discussion here so that a person 
who does not work at Cloudera can help review your code and understand all the 
jargon like "pushed from CDH5" and "cauldron".


-- 
To view, visit http://gerrit.cloudera.org:8080/7581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 74:   # We need to add additional arguments for GCC 7+. We go down this 
branch if building
> It seems ok - the memory region written is large enough. The function_buffe
I'm not thrilled to be turning off this warning. I expect it will be hard to 
turn it back on. What if we switched boost::function to std::function before 
this patch?


-- 
To view, visit http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 18:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5717/18/LICENSE.txt
File LICENSE.txt:

Line 825:   3. All advertising materials mentioning features or use of this
> I'm following Apache Kudu's lead (https://github.com/apache/kudu/blob/maste
1. Maybe it's OK, with NOTICE, as you mention, following 
https://lists.apache.org/thread.html/e0b6be4d5a507dfca727621f4ac798bed81d2dd6e24e10ff4ecb7a69@%3Clegal-discuss.apache.org%3E


-- 
To view, visit http://gerrit.cloudera.org:8080/5717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 18:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5717/18/LICENSE.txt
File LICENSE.txt:

Line 825:   3. All advertising materials mentioning features or use of this
I don't see that here:

http://www.apache.org/legal/resolved.html

or here:

https://issues.apache.org/jira/browse/LEGAL-256?jql=project%20%3D%20LEGAL%20AND%20text%20~%20%22openssl%22

Are you sure this is OK for Apache?

2. What "advertising" material are we going to have to change?


-- 
To view, visit http://gerrit.cloudera.org:8080/5717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5708: Test failure with invalid exec summary

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5708: Test failure with invalid exec summary
..


Patch Set 2:

GVD is going to fail. I think you need to rebase: 
https://jenkins.impala.io/view/Utility/job/ubuntu-16.04-from-scratch/46/

-- 
To view, visit http://gerrit.cloudera.org:8080/7627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id52ac62da2b01f9e163e97cbe4590f8db6b663d2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5773: Correctly account for memory used in data stream receiver queue

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-5773: Correctly account for memory used in data stream 
receiver queue
..


Patch Set 4:

> Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1036/

https://jenkins.impala.io/view/Utility/job/ubuntu-16.04-from-scratch/43/ 
Failed. I think this might need a rebase.

-- 
To view, visit http://gerrit.cloudera.org:8080/7646
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e90f9596ee984438e3373af05e84d361702ca6a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import  Kudu security library from 
kudu@314c9d8
..


Patch Set 14:

> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1032/

This was a bug in a Jenkins job. I fixed it (I think) and restarted the GVD.

-- 
To view, visit http://gerrit.cloudera.org:8080/5716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 17:

> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1033/

This was a bug in a Jenkins job. I fit it and restarted the GVD.

-- 
To view, visit http://gerrit.cloudera.org:8080/5717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] PREVIEW: IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: PREVIEW: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 6:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 73: if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0)
> My thought was that it's only setting GCC flags, so it shouldn't matter.
I find it is a confusing pattern.


Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
> Added a link to the Boost fix for it.
Are those warnings really spurious? I can't tell immediately from the link.

That's not to say they need to be fixed in this commit, but only that if these 
warnings are serious, we should file a ticket.


Line 75:   #   TODO: remove when we upgrade Boost: 
https://github.com/boostorg/function/pull/9
> Filed KUDU-2094 against Kudu. It looks like Counter contains class Cell, wh
Thanks for looking into that.

I'm torn on whether this patch should hold on getting these two issues (boost 
and kudu) fixed. On the one hand, waiting would allow you to forgo adding the 
-Wno- flags, and would ensure we don't accidentally leave them in forever, 
obscuring legit warnings.

OTOH, a patch this large can become outdated and difficult to commit cleanly if 
it has to wait. What do you think?


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

Line 443:   /// the
fix wrap


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:

Line 71:   Status status = impala->SetCatalogInitialized();
const


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/parquet-reader.cc
File be/src/util/parquet-reader.cc:

Line 138: // Returns the number of rows specified by the header.
pre-existing issue, but: can you add a note that this function can exit(1) or 
abort when errors are encountered?


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

Line 748:   MakeScopeExitTrigger([]() { compressor->Close(); });
Should Close() be added to the destructor (after making it unipotent)?


http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 268:   Status SerializeToArchiveString(std::string* out) const 
WARN_UNUSED_RESULT;
Makes the case for a Maybe template class that can hold a T or a Status. 
Shall we file a ticket for this, or do you feel it is too niche?


-- 
To view, visit http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW: IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: PREVIEW: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 5:

> (14 comments)

It looks like you may have forgotten to push your new patch set?

-- 
To view, visit http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5791: Make bootstrap development.sh survive apt-get failure

[Jim Apple (Code Review)]

Jim Apple has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7657

Change subject: IMPALA-5791: Make bootstrap_development.sh survive apt-get 
failure
..

IMPALA-5791: Make bootstrap_development.sh survive apt-get failure

This patch adds quadratic backoff to apt-get, which can fail for
network reasons.

While I'm here, fix bug in call to diff: diff returns failure when
there is a difference, but it is used in this script only for the
output, not the return value.

Change-Id: I2829064c9f27c39f13529b3d1a1e4f7d7a0e0128
---
M bin/bootstrap_development.sh
1 file changed, 22 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/7657/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2829064c9f27c39f13529b3d1a1e4f7d7a0e0128
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 6:

> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1029/

https://issues.apache.org/jira/browse/IMPALA-5765

-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] PREVIEW: IMPALA-2615: support [[nodiscard]] on Status

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: PREVIEW: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 5:

(14 comments)

Halfway through reviewing; though we should hash some of this out before I do 
the second half.

http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 73: if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0)
Should this be limited to GCC, too?


Line 74:   #  -Wno-placement-new: avoid spurious warnings from boost::function.
Can you describe this error a bit more?


Line 75:   #  -faligned-new: new will automatically align types. Otherwise "new 
Counter()" in the
It is my understanding that new produces things that are as aligned as void * 
(aka 8 bytes). Can you describe a bit more what's going on with this warning?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.cc
File be/src/exec/hbase-table-scanner.cc:

Line 764: discard_result(JniUtil::FreeGlobalRef(env, resultscanner_));
What is the rationale for the difference between this line and 761, where you 
LOG(WARNING)? See also below.


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/exec/hbase-table-scanner.h
File be/src/exec/hbase-table-scanner.h:

Line 88:   static Status Init() WARN_UNUSED_RESULT;
Once we move to GCC7, we no longer need WARN_UNUSED_RESULT?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:

Line 51:   /// that
fix wrap


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:

Line 40:   if (!init_status_.ok()) return init_status_;
RETURN_IF_ERROR?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:

Line 161:   if (!init_status_.ok()) return;
and LOG?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

Line 346:   Status status = mgr_->DeregisterRecvr(fragment_instance_id(), 
dest_node_id());
const


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

Line 100:   discard_result(WaitForPrepare());  // make sure Prepare() finished
Why is discard OK here?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/runtime/hbase-table-factory.cc
File be/src/runtime/hbase-table-factory.cc:

Line 95: discard_result(JniUtil::FreeGlobalRef(env, connection_));
Why is discard OK here?


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/scheduling/scheduler-test-util.cc
File be/src/scheduling/scheduler-test-util.cc:

Line 510:   Status status = scheduler_->Init();
const


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

Line 72:   THROW_IF_ERROR(LlvmCodeGen::InitializeLlvm(true), env, 
JniUtil::internal_exc_class());
I'm surprised you want to throw in a function with extern "C" linkage.


http://gerrit.cloudera.org:8080/#/c/7253/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 697: Status status = profile_logger_->Flush();
const


-- 
To view, visit http://gerrit.cloudera.org:8080/7253
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 6: Code-Review+2

rebase carry +2

-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 5: Code-Review+1

(1 comment)

Carry +1s from Michael B. and Lars.

http://gerrit.cloudera.org:8080/#/c/7587/4/bin/bootstrap_development.sh
File bin/bootstrap_development.sh:

PS4, Line 23:  and im
> double-word
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Hello Michael Brown, Lars Volker,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7587

to look at the new patch set (#5).

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..

IMPALA-4407: Move Impala setup procedures to main repo

Before this change, Impala has relied on a chef setup in
https://github.com/awleblang/impala-setup for setting up a development
environment. This has a number of downsides:

1. It makes understanding what the script is doing difficult: there
are 40k or so lines in that repo last I checked.

2. It makes porting to new distributions difficult unless the
providers of various chef "recipes" have already ported their code.

3. It makes coordinated changes between the main repo and the
impala-setup repo more awkward.

This patch adds a shell script to replace that repo. It works on
Ubuntu 14.04 and 16.04, while impala-setup repo only works on 14.04
and the now-unmaintained Ubuntu 15.04.

Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
---
M bin/bootstrap_development.sh
1 file changed, 165 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/7587/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7587/3/bin/bootstrap_development.sh
File bin/bootstrap_development.sh:

PS3, Line 21: system
: # configurations, so it is best to run this in a fresh install. 
It also sets up the
: # .bashrc of the calling user with some environment variables
> I know that the impala-setup rep didn't do this, but do you think it would 
Done


Line 57:  maven ninja-build ntp ntpdate python-dev python-setuptools 
postgresql
> Please add the following too:
Done


PS3, Line 90: # IMPALA-3932, IMPALA-3926
: SET_LD_LIBRARY_PATH='export 
LD_LIBRARY_PATH=/usr/lib/x86_64-linux-gnu/${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}'
: echo "$SET_LD_LIBRARY_PATH" >> ~/.bashrc
: eval "$SET_LD_LIBRARY_PATH"
> 1. Is this needed for Ubuntu 14? I don't do this on my personal Ubuntu 14 s
1. Fixed

2. Agreed. While we could grep .bashrc for this, that will sometimes show it as 
being already done when, in fact, changes that are below that line in the 
.bashrc override it. I have moved the output location to 
impala-config.local.sh, but I think the same applies.


PS3, Line 96: sudo -u postgres psql -c "CREATE ROLE hiveuser LOGIN PASSWORD 
'password';" postgres
> If you run this script twice, this line always fails. Could this be enhance
Done


PS3, Line 115: echo "NoHostAuthenticationForLocalhost yes" >> ~/.ssh/config
> It might be polite to have a prompt for this since it's a security change.
Added one at the top


PS3, Line 118: echo "127.0.0.1 $(hostname -s) $(hostname)" | sudo tee -a 
/etc/hosts
 : sudo sed -i 's/127.0.1.1/127.0.0.1/g' /etc/hosts
> This will keep appending to /etc/hosts if you run the script more than once
I agree. See above about .bashrc, though


PS3, Line 124: echo "* - nofile 1048576" | sudo tee -a /etc/security/limits.conf
> 1. This keeps appending to limits.conf
1. See above
2. Added #TODO. It can't just be $(whoami), because it might need that for 
postgres or root.


PS3, Line 127: export IMPALA_HOME="$(pwd)"
> What do you think about setting this in .bashrc also?
Done


PS3, Line 131: git clone https://github.com/cloudera/impala-lzo.git
 : ln -s impala-lzo Impala-lzo
 : git clone https://github.com/cloudera/hadoop-lzo.git
> This fails if you run the script twice. Could you add -d tests here similar
Done


PS3, Line 139: if [[ $VERSION = "14.04" ]]
 : then
 :   unset LD_LIBRARY_PATH
 : fi
> Oh, this sort of answers my question above. What about when the user create
Fixed


PS3, Line 145: time -p ./buildall.sh -noclean -format -testdata
> I'm torn about this, because it takes hours. Maybe consider ./buildall.sh -
Changed so it doesn't run tests - I think it might be confusing otherwise to 
users who think they have bootstrapped but then can't run a test because the 
data isn't loaded, especially because loading data is somewhat uncommon in my 
experience as a step to get a working dev environment (among other open source 
projects).this


-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Hello Lars Volker,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7587

to look at the new patch set (#4).

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..

IMPALA-4407: Move Impala setup procedures to main repo

Before this change, Impala has relied on a chef setup in
https://github.com/awleblang/impala-setup for setting up a development
environment. This has a number of downsides:

1. It makes understanding what the script is doing difficult: there
are 40k or so lines in that repo last I checked.

2. It makes porting to new distributions difficult unless the
providers of various chef "recipes" have already ported their code.

3. It makes coordinated changes between the main repo and the
impala-setup repo more awkward.

This patch adds a shell script to replace that repo. It works on
Ubuntu 14.04 and 16.04, while impala-setup repo only works on 14.04
and the now-unmaintained Ubuntu 15.04.

Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
---
M bin/bootstrap_development.sh
1 file changed, 165 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/7587/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5484: Fix LICENSE issues discovered by IPMC in 2.9 vote

[Jim Apple (Code Review)]

Jim Apple has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7600

Change subject: IMPALA-5484: Fix LICENSE issues discovered by IPMC in 2.9 vote
..

IMPALA-5484: Fix LICENSE issues discovered by IPMC in 2.9 vote

Change-Id: I0f98d1b2f514d7afdee8d86a45167905b272ca4d
---
M LICENSE.txt
1 file changed, 3 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/7600/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7600
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f98d1b2f514d7afdee8d86a45167905b272ca4d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7587/2/bin/bootstrap_development.sh
File bin/bootstrap_development.sh:

Line 22: # configurations, so it is best to run this in a fresh install.
> I was more thinking of "what if the script fails after step X?" - is it pos
The only steps that are not idempotent are "CREATE ROLE hiveuser LOGIN PASSWORD 
'password'" and the git checkouts of the lzo directories. and all of the steps 
but the last take a very small amount of time (two minutes total out of 240 
minutes).

So, no matter where you are, if you undo those steps, you can start from 
scratch with minimal loss of time.


-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7587/2/bin/bootstrap_development.sh
File bin/bootstrap_development.sh:

Line 22: # configurations, so it is best to run this in a fresh install.
> Can you add a comment whether it's possible to run this again, e.g. in case
Done


Line 30: # exploration mode.
> Do we have an umbrella Jenkins that we use to test changes made to this scr
Since Jenkins jobs are configured away from this script, I'd prefer to keep the 
comments separate to avoid false dependencies: if someone deletes a Jenkins 
job, I don't want the comment here to grow stale.

In any case, the only such job is an experimental one: 
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/


PS2, Line 34: grep "Ubuntu"
> nit: "grep -q" prevents printing the match.
I'm OK with it printing. For a batch bash script, and one that runs the tests 
for a project like Impala with a dense history of flaky tests, flaky 
bootstrapping, and flaky builds, I think verbosity in the output is a virtue.


PS2, Line 36: echo "This script only supports Ubuntu" >&2
:   exit 1
> You could define a helper die() that does the echo and exit 1. Then you cou
I thin that would actually add lines and complexity.


PS2, Line 66: sudo service ntp restart
> Why is this line needed?
Done


PS2, Line 70: grep amazon
> You should check for the existence of dmicode before calling it, i.e. if wh
Done


PS2, Line 73:  grep amazon /etc/ntp.conf
:   grep ubuntu /etc/ntp.conf
> Are these for debugging purposes?
yes.


Line 88:   SET_LD_LIBRARY_PATH="$SET_LD_LIBRARY_PATH:"'$LD_LIBRARY_PATH'
> Consider using something like the following. That way we'll maintain LD_LIB
Done


Line 90: echo "$SET_LD_LIBRARY_PATH" >> ~/.bashrc
> I'm a bit concerned that this does not work on zsh systems, but I also don'
Done


Line 108:   ssh-keygen -t rsa -N '' -q -f ~/.ssh/id_rsa
> We should mkdir -p ~/.ssh and chmod 600 it, it looks like ssh-keygen wont' 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#3).

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..

IMPALA-4407: Move Impala setup procedures to main repo

Before this change, Impala has relied on a chef setup in
https://github.com/awleblang/impala-setup for setting up a development
environment. This has a number of downsides:

1. It makes understanding what the script is doing difficult: there
are 40k or so lines in that repo last I checked.

2. It makes porting to new distributions difficult unless the
providers of various chef "recipes" have already ported their code.

3. It makes coordinated changes between the main repo and the
impala-setup repo more awkward.

This patch adds a shell script to replace that repo. It works on
Ubuntu 14.04 and 16.04, while impala-setup repo only works on 14.04
and the now-unmaintained Ubuntu 15.04.

Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
---
M bin/bootstrap_development.sh
1 file changed, 100 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/7587/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2).

Change subject: IMPALA-4407: Move Impala setup procedures to main repo
..

IMPALA-4407: Move Impala setup procedures to main repo

Before this change, Impala has relied on a chef setup in
https://github.com/awleblang/impala-setup for setting up a development
environment. This has a number of downsides:

1. It makes understanding what the script is doing difficult: there
are 40k or so lines in that repo last I checked.

2. It makes porting to new distributions difficult unless the
providers of various chef "recipes" have already ported their code.

3. It makes coordinated changes between the main repo and the
impala-setup repo more awkward.

This patch adds a shell script to replace that repo. It works on
Ubuntu 14.04 and 16.04, while impala-setup repo only works on 14.04
and the now-unmaintained Ubuntu 15.04.

Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
---
M bin/bootstrap_development.sh
1 file changed, 97 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/7587/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong