impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4047: DO NOT SUBMIT Highlight all occurrences of CDH/cdh
Date Thu, 29 Sep 2016 11:22:39 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-4047: DO NOT SUBMIT Highlight all occurrences of CDH/cdh
......................................................................


Patch Set 5:

(24 comments)

Thanks for all the help so far. I created a bunch of upstream Jiras and replaced references
to internal ones.

Please see my inline comments.

http://gerrit.cloudera.org:8080/#/c/4187/4/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

PS4, Line 24: 
Can we rename this to DOWNLOAD_HADOOP_COMPONENTS instead? Or are they CDH specific versions
and that would make it worse?


http://gerrit.cloudera.org:8080/#/c/4187/4/bin/build_thirdparty.sh
File bin/build_thirdparty.sh:

Line 46
This was unused, removed


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

PS1, Line 79: 
Do we have plans to actually get rid of the CDH component dependencies? This one seems to
affect a lot more occurrences below. If anyone can shed some light on it I'll go ahead and
create a Jira for doing it.


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

Line 144
> There are so many different things we are doing with different "CDH"s, it m
Created IMPALA-4208 to track this.


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

PS4, Line 77: 
see comments in bootstrap_toolchain.py


http://gerrit.cloudera.org:8080/#/c/4187/4/bin/save-version.sh
File bin/save-version.sh:

Line 24
This has been fixed separately in IMPALA-4116


http://gerrit.cloudera.org:8080/#/c/4187/4/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

PS4, Line 220: 
             : 
IMPALA-4208


http://gerrit.cloudera.org:8080/#/c/4187/1/common/thrift/generate_metrics.py
File common/thrift/generate_metrics.py:

PS1, Line 49: parser.add_option("--output_mdl_version", dest="output_mdl_version",
            :                   metavar="IMPALA_VERSION", default="2.8.0-SNAPSHOT",
            :                   help="The Impala version that is written in the output mdl.")
> Thanks. Let me know what they say and I'll create a Jira to track removal o
Did anything come out of this? For now I changed the version to the current version in save-version.sh,
so the 'cdh' is gone. :)


http://gerrit.cloudera.org:8080/#/c/4187/4/common/thrift/generate_metrics.py
File common/thrift/generate_metrics.py:

Line 50
I think we should have better scripts for version management, i.e. one script that produces
proper version files for all languages to include. For now I will bump this to the current
version in save-version.sh.


http://gerrit.cloudera.org:8080/#/c/4187/4/ext-data-source/api/pom.xml
File ext-data-source/api/pom.xml:

IMPALA-4225


http://gerrit.cloudera.org:8080/#/c/4187/4/ext-data-source/sample/pom.xml
File ext-data-source/sample/pom.xml:

IMPALA-4225


http://gerrit.cloudera.org:8080/#/c/4187/4/ext-data-source/test/pom.xml
File ext-data-source/test/pom.xml:

IMPALA-4225


http://gerrit.cloudera.org:8080/#/c/4187/4/fe/pom.xml
File fe/pom.xml:

IMPALA-4225


http://gerrit.cloudera.org:8080/#/c/4187/4/infra/deploy/deploy.py
File infra/deploy/deploy.py:

PS4, Line 23: 
I doubt this is accurate, otherwise it could probably be removed, since this quite an old
version.

@mj, can you have a look? Should we move this script to some internal repo or is it useful
without CDH, too?

Let me know what to do and I'll open a Jira to track.


http://gerrit.cloudera.org:8080/#/c/4187/3/testdata/cluster/.gitignore
File testdata/cluster/.gitignore:

PS3, Line 1: kud
> I'm not sure about this one. I don't want to have to add one of these for e
Good point, removed it.


http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/cluster/admin
File testdata/cluster/admin:

> Yes, I think I was wrong. There was a similar discussion in bin/start-impal
IMPALA-4208


http://gerrit.cloudera.org:8080/#/c/4187/4/testdata/cluster/admin
File testdata/cluster/admin:

IMPALA-4208


PS4, Line 28: 
            : 
IMPALA-4208


http://gerrit.cloudera.org:8080/#/c/4187/4/testdata/datasets/functional/schema_constraints.csv
File testdata/datasets/functional/schema_constraints.csv:

PS4, Line 126: 
Wouldn't this hold true for all versions of CDH (and all environments, really)? Then again
I don't understand why the comment is there in the first place. Can decimal not be tested
read-only?


http://gerrit.cloudera.org:8080/#/c/4187/4/tests/comparison/cluster.py
File tests/comparison/cluster.py:

IMPALA-4208


http://gerrit.cloudera.org:8080/#/c/4187/3/tests/comparison/leopard/impala_docker_env.py
File tests/comparison/leopard/impala_docker_env.py:

Line 34: DEFAULT_BRANCH_NAME = 'origin/cdh5-trunk'
> Whether IMPALA-4085 is fixed, we should remove these.
I don't know, I will ask on the mailing list.

By "remove these", do you mean the variables? Or the whole file?


http://gerrit.cloudera.org:8080/#/c/4187/4/tests/metadata/test_compute_stats.py
File tests/metadata/test_compute_stats.py:

PS4, Line 45: 
I reworded this.


http://gerrit.cloudera.org:8080/#/c/4187/4/tests/query_test/test_decimal_queries.py
File tests/query_test/test_decimal_queries.py:

Line 38
This seems to be Hive < 0.11, so I will reword this.


http://gerrit.cloudera.org:8080/#/c/4187/4/tests/test-hive-udfs/pom.xml
File tests/test-hive-udfs/pom.xml:

IMPALA-4225


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icb37e2ef0cd9fa0e581d359c5dd3db7812b7b2c8
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message